Re: [PATCH, PR 42398] More generic type compatibility checking in sra_modify_expr

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Dec 22, 2009 at 9:48 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> the testcase in PR 42398 persuaded me that we need fully generic type
> compatibility checking in sra_modify_expr rather than looking for
> potential causes of incompatibilities in situations that are handled
> by the function. ?The downside is that the type conversion code leaves
> the old aggregate around (it has to for TREE_ADDRESSABLE types) and we
> will not be drawn to new causes of it being used by the assert if any
> occur.
>
> The alternative would be to add code generating new temporaries and
> then an assignment from a VIEW_CONVERT_EXPRs but that hardly seems
> worth it for assembly statements.
>
> The patch leaves flag grp_different_types useless and so I removed it
> so that it is not calculated needlessly.
>
> I have successfully bootstrapped and regression tested the patch on
> x86_64-linux. ?OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>
> 2009-12-22 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?PR tree-optimization/42398
> ? ? ? ?* tree-sra.c (struct access): Removed flag grp_different_types.
> ? ? ? ?(dump_access): Do not dump the removed flag.
> ? ? ? ?(sort_and_splice_var_accesses): Do not set the removed flag.
> ? ? ? ?(sra_modify_expr): Check for type compatibility directly.
>
> ? ? ? ?* testsuite/gcc.c-torture/compile/pr42398.c: New test.
>
>
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -199,10 +199,6 @@ struct access
> ? ? ?BIT_FIELD_REF? ?*/
> ? unsigned grp_partial_lhs : 1;
>
> - ?/* Does this group contain accesses to different types? (I.e. through a union
> - ? ? or a similar mechanism). ?*/
> - ?unsigned grp_different_types : 1;
> -
> ? /* Set when a scalar replacement should be created for this variable. ?We do
> ? ? ?the decision and creation at different places because create_tmp_var
> ? ? ?cannot be called from within FOR_EACH_REFERENCED_VAR. */
> @@ -343,14 +339,12 @@ dump_access (FILE *f, struct access *acc
> ? ? fprintf (f, ", grp_write = %d, grp_read = %d, grp_hint = %d, "
> ? ? ? ? ? ? "grp_covered = %d, grp_unscalarizable_region = %d, "
> ? ? ? ? ? ? "grp_unscalarized_data = %d, grp_partial_lhs = %d, "
> - ? ? ? ? ? ?"grp_different_types = %d, grp_to_be_replaced = %d, "
> - ? ? ? ? ? ?"grp_maybe_modified = %d, "
> + ? ? ? ? ? ?"grp_to_be_replaced = %d, grp_maybe_modified = %d, "
> ? ? ? ? ? ? "grp_not_necessarilly_dereferenced = %d\n",
> ? ? ? ? ? ? access->grp_write, access->grp_read, access->grp_hint,
> ? ? ? ? ? ? access->grp_covered, access->grp_unscalarizable_region,
> ? ? ? ? ? ? access->grp_unscalarized_data, access->grp_partial_lhs,
> - ? ? ? ? ? ?access->grp_different_types, access->grp_to_be_replaced,
> - ? ? ? ? ? ?access->grp_maybe_modified,
> + ? ? ? ? ? ?access->grp_to_be_replaced, access->grp_maybe_modified,
> ? ? ? ? ? ? access->grp_not_necessarilly_dereferenced);
> ? else
> ? ? fprintf (f, ", write = %d, grp_partial_lhs = %d\n", access->write,
> @@ -1434,7 +1428,6 @@ sort_and_splice_var_accesses (tree var)
> ? ? ? bool grp_read = !access->write;
> ? ? ? bool multiple_reads = false;
> ? ? ? bool grp_partial_lhs = access->grp_partial_lhs;
> - ? ? ?bool grp_different_types = false;
> ? ? ? bool first_scalar = is_gimple_reg_type (access->type);
> ? ? ? bool unscalarizable_region = access->grp_unscalarizable_region;
>
> @@ -1466,7 +1459,6 @@ sort_and_splice_var_accesses (tree var)
> ? ? ? ? ? ? ? ?grp_read = true;
> ? ? ? ? ? ?}
> ? ? ? ? ?grp_partial_lhs |= ac2->grp_partial_lhs;
> - ? ? ? ? grp_different_types |= !types_compatible_p (access->type, ac2->type);
> ? ? ? ? ?unscalarizable_region |= ac2->grp_unscalarizable_region;
> ? ? ? ? ?relink_to_new_repr (access, ac2);
>
> @@ -1485,7 +1477,6 @@ sort_and_splice_var_accesses (tree var)
> ? ? ? access->grp_read = grp_read;
> ? ? ? access->grp_hint = multiple_reads;
> ? ? ? access->grp_partial_lhs = grp_partial_lhs;
> - ? ? ?access->grp_different_types = grp_different_types;
> ? ? ? access->grp_unscalarizable_region = unscalarizable_region;
> ? ? ? if (access->first_link)
> ? ? ? ?add_access_to_work_queue (access);
> @@ -2136,11 +2127,9 @@ sra_modify_expr (tree *expr, gimple_stmt
>
> ? ? ? ? ?We also want to use this when accessing a complex or vector which can
> ? ? ? ? ?be accessed as a different type too, potentially creating a need for
> - ? ? ? ? type conversion ?(see PR42196). ?*/
> - ? ? ?if (!is_gimple_reg_type (type)
> - ? ? ? ? || (access->grp_different_types
> - ? ? ? ? ? ? && (TREE_CODE (type) == COMPLEX_TYPE
> - ? ? ? ? ? ? ? ? || TREE_CODE (type) == VECTOR_TYPE)))
> + ? ? ? ? type conversion (see PR42196) and when scalarized unions are involved
> + ? ? ? ? in assembler statements (see PR42398). ?*/
> + ? ? ?if (!useless_type_conversion_p (type, access->type))
> ? ? ? ?{
> ? ? ? ? ?tree ref = access->base;
> ? ? ? ? ?bool ok;
> @@ -2171,10 +2160,7 @@ sra_modify_expr (tree *expr, gimple_stmt
> ? ? ? ? ? ?}
> ? ? ? ?}
> ? ? ? else
> - ? ? ? {
> - ? ? ? ? gcc_assert (useless_type_conversion_p (type, access->type));
> - ? ? ? ? *expr = repl;
> - ? ? ? }
> + ? ? ? *expr = repl;
> ? ? ? sra_stats.exprs++;
> ? ? }
>
> Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42398.c
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gcc.c-torture/compile/pr42398.c
> @@ -0,0 +1,6 @@
> +int ptrace_setregs(void)
> +{
> + ?union { unsigned int l; int t; } __gu_tmp;
> + ?__asm__ __volatile__("" : "=r" (__gu_tmp.l));
> + ?return __gu_tmp.t;
> +}
>
>