[PATCH, PR 40493] Fix SRA miscompilation of binutils
Richard Guenther
richard.guenther@gmail.com
Wed Jun 24 22:38:00 GMT 2009
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;
> +}
>
More information about the Gcc-patches
mailing list