Re: [PATCH, PR 40493] Fix SRA miscompilation of binutils

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

2009/6/24 Martin Jambor <mjambor@suse.cz>:
> Hi,
>
> the patch below fixes two rather serious problems in the new SRA.
>
> The ?first one ?is ?that I ?misremembered ?the order ?of arguments ?of
> BIT_FIELD_REF and was looking for the offset and size of the reference
> at exactly the wrong places.
>
> The second ?is a slightly ?more complex one. ? sra_modify_assign() and
> load_assign_lhs_subreplacements() ?try hard ?to ?remove the ?aggregate
> assignment ?if ?possible ?and ?so ?can ?decide ?to ?flush ?RHS ?scalar
> replacements ?directly to ?the ?LHS ?when it ?knows ?there's no ?other
> (unscalarized) data on the RHS. ?That is all good and well except that
> load_assign_lhs_subreplacements() always ?looked to the ?RHS aggregate
> when it did ?not find a RHS scalar replacement ?corresponding to a LHS
> scalar replacement. ?However, that contained
>
> This patch fixes ?this changes a boolean variable ?that keeps track of
> whether scalars were flushed into one of the original aggregates to an
> enum that also tells to which one and makes
> load_assign_lhs_subreplacements use it to look at the correct place.
>
> I have bootstrapped ?and tested this on x86-64. ? There was however an
> acats new ?failure which has something ?to do with ?timing and delays.
> However, these happen ?to me all the ?time but go away when ?I run the
> tests again and so that's what I am doing right now.

A usual problem, you can just ignore these.

> So, is this OK provided that the acats failure does not reoccur?

Ok.
Thanks,
Richard.

> Thanks,
>
> Martin
>
> 2009-06-24 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?* tree-sra.c (sra_modify_expr): Correct BIT_FIELD_REF argument numbers.
> ? ? ? ?(enum unscalarized_data_handling): New type.
> ? ? ? ?(handle_unscalarized_data_in_subtree): Return what has been done.
> ? ? ? ?(load_assign_lhs_subreplacements): Handle left flushes differently.
> ? ? ? ?(sra_modify_assign): Use unscalarized_data_handling, simplified
> ? ? ? ?condition determining whether to remove the statement.
>
> ? ? ? ?* testsuite/gcc.c-torture/execute/pr40493.c: New test.
>
>
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -1907,8 +1907,8 @@ sra_modify_expr (tree *expr, gimple_stmt
> ? ? ? ? ?&& host_integerp (TREE_OPERAND (bfr, 1), 1)
> ? ? ? ? ?&& host_integerp (TREE_OPERAND (bfr, 2), 1))
> ? ? ? ?{
> - ? ? ? ? start_offset = tree_low_cst (TREE_OPERAND (bfr, 1), 1);
> - ? ? ? ? chunk_size = tree_low_cst (TREE_OPERAND (bfr, 2), 1);
> + ? ? ? ? chunk_size = tree_low_cst (TREE_OPERAND (bfr, 1), 1);
> + ? ? ? ? start_offset = tree_low_cst (TREE_OPERAND (bfr, 2), 1);
> ? ? ? ?}
> ? ? ? else
> ? ? ? ?start_offset = chunk_size = 0;
> @@ -1919,20 +1919,33 @@ sra_modify_expr (tree *expr, gimple_stmt
> ? return true;
> ?}
>
> +/* Where scalar replacements of the RHS have been written to when a replacement
> + ? of a LHS of an assigments cannot be direclty loaded from a replacement of
> + ? the RHS. */
> +enum unscalarized_data_handling { SRA_UDH_NONE, ?/* Nothing done so far. */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SRA_UDH_RIGHT, /* Data flushed to the RHS. */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SRA_UDH_LEFT }; /* Data flushed to the LHS. */
> +
> ?/* Store all replacements in the access tree rooted in TOP_RACC either to their
> ? ?base aggregate if there are unscalarized data or directly to LHS
> ? ?otherwise. ?*/
>
> -static void
> +static enum unscalarized_data_handling
> ?handle_unscalarized_data_in_subtree (struct access *top_racc, tree lhs,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gimple_stmt_iterator *gsi)
> ?{
> ? if (top_racc->grp_unscalarized_data)
> - ? ?generate_subtree_copies (top_racc->first_child, top_racc->base, 0, 0, 0,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ?gsi, false, false);
> + ? ?{
> + ? ? ?generate_subtree_copies (top_racc->first_child, top_racc->base, 0, 0, 0,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gsi, false, false);
> + ? ? ?return SRA_UDH_RIGHT;
> + ? ?}
> ? else
> - ? ?generate_subtree_copies (top_racc->first_child, lhs, top_racc->offset,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ?0, 0, gsi, false, false);
> + ? ?{
> + ? ? ?generate_subtree_copies (top_racc->first_child, lhs, top_racc->offset,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?0, 0, gsi, false, false);
> + ? ? ?return SRA_UDH_LEFT;
> + ? ?}
> ?}
>
>
> @@ -1951,7 +1964,8 @@ load_assign_lhs_subreplacements (struct
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? HOST_WIDE_INT right_offset,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gimple_stmt_iterator *old_gsi,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gimple_stmt_iterator *new_gsi,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?bool *refreshed, tree lhs)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum unscalarized_data_handling *refreshed,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?tree lhs)
> ?{
> ? do
> ? ? {
> @@ -1975,18 +1989,20 @@ load_assign_lhs_subreplacements (struct
>
> ? ? ? ? ? ? ?/* No suitable access on the right hand side, need to load from
> ? ? ? ? ? ? ? ? the aggregate. ?See if we have to update it first... */
> - ? ? ? ? ? ? if (!*refreshed)
> + ? ? ? ? ? ? if (*refreshed == SRA_UDH_NONE)
> + ? ? ? ? ? ? ? *refreshed = handle_unscalarized_data_in_subtree (top_racc,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lhs, old_gsi);
> +
> + ? ? ? ? ? ? if (*refreshed == SRA_UDH_LEFT)
> + ? ? ? ? ? ? ? rhs = unshare_expr (lacc->expr);
> + ? ? ? ? ? ? else
> ? ? ? ? ? ? ? ?{
> - ? ? ? ? ? ? ? ? gcc_assert (top_racc->first_child);
> - ? ? ? ? ? ? ? ? handle_unscalarized_data_in_subtree (top_racc, lhs, old_gsi);
> - ? ? ? ? ? ? ? ? *refreshed = true;
> + ? ? ? ? ? ? ? ? rhs = unshare_expr (top_racc->base);
> + ? ? ? ? ? ? ? ? repl_found = build_ref_for_offset (&rhs,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TREE_TYPE (top_racc->base),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?offset, lacc->type, false);
> + ? ? ? ? ? ? ? ? gcc_assert (repl_found);
> ? ? ? ? ? ? ? ?}
> -
> - ? ? ? ? ? ? rhs = unshare_expr (top_racc->base);
> - ? ? ? ? ? ? repl_found = build_ref_for_offset (&rhs,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TREE_TYPE (top_racc->base),
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?offset, lacc->type, false);
> - ? ? ? ? ? ? gcc_assert (repl_found);
> ? ? ? ? ? ?}
>
> ? ? ? ? ?stmt = gimple_build_assign (get_access_replacement (lacc), rhs);
> @@ -1994,11 +2010,10 @@ load_assign_lhs_subreplacements (struct
> ? ? ? ? ?update_stmt (stmt);
> ? ? ? ? ?sra_stats.subreplacements++;
> ? ? ? ?}
> - ? ? ?else if (lacc->grp_read && !lacc->grp_covered && !*refreshed)
> - ? ? ? {
> - ? ? ? ? handle_unscalarized_data_in_subtree (top_racc, lhs, old_gsi);
> - ? ? ? ? *refreshed = true;
> - ? ? ? }
> + ? ? ?else if (*refreshed == SRA_UDH_NONE
> + ? ? ? ? ? ? ?&& lacc->grp_read && !lacc->grp_covered)
> + ? ? ? *refreshed = handle_unscalarized_data_in_subtree (top_racc, lhs,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? old_gsi);
>
> ? ? ? if (lacc->first_child)
> ? ? ? ?load_assign_lhs_subreplacements (lacc->first_child, top_racc,
> @@ -2204,20 +2219,17 @@ sra_modify_assign (gimple *stmt, gimple_
> ? ? ? if (access_has_children_p (lacc) && access_has_children_p (racc))
> ? ? ? ?{
> ? ? ? ? ?gimple_stmt_iterator orig_gsi = *gsi;
> - ? ? ? ? bool refreshed;
> + ? ? ? ? enum unscalarized_data_handling refreshed;
>
> ? ? ? ? ?if (lacc->grp_read && !lacc->grp_covered)
> - ? ? ? ? ? {
> - ? ? ? ? ? ? handle_unscalarized_data_in_subtree (racc, lhs, gsi);
> - ? ? ? ? ? ? refreshed = true;
> - ? ? ? ? ? }
> + ? ? ? ? ? refreshed = handle_unscalarized_data_in_subtree (racc, lhs, gsi);
> ? ? ? ? ?else
> - ? ? ? ? ? refreshed = false;
> + ? ? ? ? ? refreshed = SRA_UDH_NONE;
>
> ? ? ? ? ?load_assign_lhs_subreplacements (lacc->first_child, racc,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lacc->offset, racc->offset,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &orig_gsi, gsi, &refreshed, lhs);
> - ? ? ? ? if (!refreshed || !racc->grp_unscalarized_data)
> + ? ? ? ? if (refreshed != SRA_UDH_RIGHT)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?if (*stmt == gsi_stmt (*gsi))
> ? ? ? ? ? ? ? ?gsi_next (gsi);
> Index: mine/gcc/testsuite/gcc.c-torture/execute/pr40493.c
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gcc.c-torture/execute/pr40493.c
> @@ -0,0 +1,82 @@
> +extern void abort (void);
> +
> +typedef union i386_operand_type
> +{
> + ?struct
> + ? ?{
> + ? ? ?unsigned int reg8:1;
> + ? ? ?unsigned int reg16:1;
> + ? ? ?unsigned int reg32:1;
> + ? ? ?unsigned int reg64:1;
> + ? ? ?unsigned int floatreg:1;
> + ? ? ?unsigned int regmmx:1;
> + ? ? ?unsigned int regxmm:1;
> + ? ? ?unsigned int regymm:1;
> + ? ? ?unsigned int control:1;
> + ? ? ?unsigned int debug:1;
> + ? ? ?unsigned int test:1;
> + ? ? ?unsigned int sreg2:1;
> + ? ? ?unsigned int sreg3:1;
> + ? ? ?unsigned int imm1:1;
> + ? ? ?unsigned int imm8:1;
> + ? ? ?unsigned int imm8s:1;
> + ? ? ?unsigned int imm16:1;
> + ? ? ?unsigned int imm32:1;
> + ? ? ?unsigned int imm32s:1;
> + ? ? ?unsigned int imm64:1;
> + ? ? ?unsigned int disp8:1;
> + ? ? ?unsigned int disp16:1;
> + ? ? ?unsigned int disp32:1;
> + ? ? ?unsigned int disp32s:1;
> + ? ? ?unsigned int disp64:1;
> + ? ? ?unsigned int acc:1;
> + ? ? ?unsigned int floatacc:1;
> + ? ? ?unsigned int baseindex:1;
> + ? ? ?unsigned int inoutportreg:1;
> + ? ? ?unsigned int shiftcount:1;
> + ? ? ?unsigned int jumpabsolute:1;
> + ? ? ?unsigned int esseg:1;
> + ? ? ?unsigned int regmem:1;
> + ? ? ?unsigned int mem:1;
> + ? ? ?unsigned int byte:1;
> + ? ? ?unsigned int word:1;
> + ? ? ?unsigned int dword:1;
> + ? ? ?unsigned int fword:1;
> + ? ? ?unsigned int qword:1;
> + ? ? ?unsigned int tbyte:1;
> + ? ? ?unsigned int xmmword:1;
> + ? ? ?unsigned int ymmword:1;
> + ? ? ?unsigned int unspecified:1;
> + ? ? ?unsigned int anysize:1;
> + ? ?} bitfield;
> + ?unsigned int array[2];
> +} i386_operand_type;
> +
> +unsigned int x00, x01, y00, y01;
> +
> +int main (int argc, char *argv[])
> +{
> + ?i386_operand_type a,b,c,d;
> +
> + ?a.bitfield.reg16 = 1;
> + ?a.bitfield.imm16 = 0;
> + ?a.array[1] = 22;
> +
> + ?b = a;
> + ?x00 = b.array[0];
> + ?x01 = b.array[1];
> +
> + ?c = b;
> + ?y00 = c.array[0];
> + ?y01 = c.array[1];
> +
> + ?d = c;
> + ?if (d.bitfield.reg16 != 1)
> + ? ?abort();
> + ?if (d.bitfield.imm16 != 0)
> + ? ?abort();
> + ?if (d.array[1] != 22)
> + ? ?abort();
> +
> + ?return 0;
> +}
>