Message ID | 20131212212513.GP892@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >Before the PR58956 fix find_replaceable_in_bb only gave up TER if >stmt was gimple_assign_single_p that was storing something that >could alias with TERed load, but now it also punts on calls (a lot of >them >apparently) and inline asm (all of them, because stmt_may_clobber_ref_p >always returns true for GIMPLE_ASM). I believe we can restore TERing >most of the cases for calls and inline asm though, because for calls >all arguments are necessarily evaluated before the call and the only >memory side effects of the call are the call itself and perhaps storing >of non-SSA_NAME return value afterwards. And for inline asm again >all input operands have to be evaluated before the inline asm, and the >only storing side-effects are again inside of the inline asm and >perhaps >storing of the output operands. > >Thus, this patch will only stop TERing if USE is used somewhere in >lhs of a call for calls or somewhere in output arguments of inline asm. Can you please simply use walk_stmt_load_store_ops to get at the stmt outputs? Thanks, Richard. >Bootstrapped/regtested on x86_64-linux and i686-linux (both on trunk >and 4.8 >branch), ok for trunk/4.8? > >The reason I'd like to see this for 4.8 is to decrease the amount of >code >generation changes from pre-r205709 to minimum, as it can uncover other >latent bugs than just PR59470. > >2013-12-12 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/58956 > PR middle-end/59470 > * tree-ssa-ter.c (find_ssa_name): New helper function. > (find_replaceable_in_bb): For calls, only set same_root_var > if USE is used somewhere in gimple_call_lhs, for GIMPLE_ASM, > only set same_root_var if USE is used somewhere in output operand > trees. > > * gcc.target/i386/pr59470.c: New test. > >--- gcc/tree-ssa-ter.c.jj 2013-12-10 08:52:13.000000000 +0100 >+++ gcc/tree-ssa-ter.c 2013-12-12 10:43:26.177866960 +0100 >@@ -554,6 +554,20 @@ mark_replaceable (temp_expr_table_p tab, > } > > >+/* Helper function for find_replaceable_in_bb. Called via walk_tree >to >+ find a SSA_NAME DATA somewhere in *TP. */ >+ >+static tree >+find_ssa_name (tree *tp, int *walk_subtrees, void *data) >+{ >+ tree var = (tree) data; >+ if (*tp == var) >+ return var; >+ else if (IS_TYPE_OR_DECL_P (*tp)) >+ *walk_subtrees = 0; >+ return NULL_TREE; >+} >+ >/* This function processes basic block BB, and looks for variables >which can >be replaced by their expressions. Results are stored in the table TAB. > */ > >@@ -618,7 +632,42 @@ find_replaceable_in_bb (temp_expr_table_ > && gimple_assign_single_p (def_stmt) > && stmt_may_clobber_ref_p (stmt, > gimple_assign_rhs1 (def_stmt))) >- same_root_var = true; >+ { >+ if (is_gimple_call (stmt)) >+ { >+ /* For calls, it is not a problem if USE is among >+ call's arguments or say OBJ_TYPE_REF argument, >+ all those necessarily need to be evaluated before >+ the call that may clobber the memory. But if >+ LHS of the call refers to USE, expansion might >+ evaluate it after the call, prevent TER in that >+ case. */ >+ if (gimple_call_lhs (stmt) >+ && TREE_CODE (gimple_call_lhs (stmt)) != SSA_NAME >+ && walk_tree (gimple_call_lhs_ptr (stmt), >+ find_ssa_name, use, NULL)) >+ same_root_var = true; >+ } >+ else if (gimple_code (stmt) == GIMPLE_ASM) >+ { >+ /* For inline asm, allow TER of loads into input >+ arguments, but disallow TER for USEs that occur >+ somewhere in outputs. */ >+ unsigned int i; >+ for (i = 0; i < gimple_asm_noutputs (stmt); i++) >+ if (TREE_CODE (gimple_asm_output_op (stmt, i)) >+ != SSA_NAME >+ && walk_tree (gimple_asm_output_op_ptr (stmt, >+ i), >+ find_ssa_name, use, NULL)) >+ { >+ same_root_var = true; >+ break; >+ } >+ } >+ else >+ same_root_var = true; >+ } > } > > /* Mark expression as replaceable unless stmt is volatile, or >the >--- gcc/testsuite/gcc.target/i386/pr59470.c.jj 2013-12-12 >10:31:54.746517544 +0100 >+++ gcc/testsuite/gcc.target/i386/pr59470.c 2013-12-12 >10:32:42.045273313 +0100 >@@ -0,0 +1,17 @@ >+/* PR middle-end/58956 */ >+/* PR middle-end/59470 */ >+/* { dg-do run } */ >+/* { dg-options "-O2" } */ >+ >+int a, b, d[1024]; >+ >+int >+main () >+{ >+ int c = a; >+ asm ("{movl $6, (%2); movl $1, %0|mov dword ptr [%2], 6; mov %0, 1}" >+ : "=r" (d[c]) : "rm" (b), "r" (&a) : "memory"); >+ if (d[0] != 1 || d[6] != 0) >+ __builtin_abort (); >+ return 0; >+} > > Jakub
On Fri, Dec 13, 2013 at 07:30:12AM +0100, Richard Biener wrote: > Jakub Jelinek <jakub@redhat.com> wrote: > >lhs of a call for calls or somewhere in output arguments of inline asm. > > Can you please simply use walk_stmt_load_store_ops to get at the stmt outputs? No, unfortunately. The problem is that walk_stmt_load_store_{addr_,}ops first calls and get_base_loadstore on the operand and thus effectively strips all the handled components from it. But we need to look at any uses of SSA_NAMEs in the whole operand, not only if it is based on *MEM_REF with SSA_NAME operand. I.e., a change of the patch to use walk_stmt_load_store_ops will keep the pr58956.c testcase fixed, because there is *i, but will make pr59470.c (the new one in the patch) broken, because there the SSA_NAME is used as ARRAY_REF index and base is some VAR_DECL. It guess it wouldn't be hard to make similar testcase even for the call case, though it is unclear if it would be miscompiled or not. Jakub
Jakub Jelinek <jakub@redhat.com> wrote: >On Fri, Dec 13, 2013 at 07:30:12AM +0100, Richard Biener wrote: >> Jakub Jelinek <jakub@redhat.com> wrote: >> >lhs of a call for calls or somewhere in output arguments of inline >asm. >> >> Can you please simply use walk_stmt_load_store_ops to get at the stmt >outputs? > >No, unfortunately. The problem is that >walk_stmt_load_store_{addr_,}ops first >calls and get_base_loadstore on the operand and thus effectively strips >all the >handled components from it. That's a defficiency of that function then which we should fix, for example with either passing both to the callback or by adding a flag. Richard. But we need to look at any uses of >SSA_NAMEs >in the whole operand, not only if it is based on *MEM_REF with SSA_NAME >operand. I.e., a change of the patch to use walk_stmt_load_store_ops >will keep the pr58956.c testcase fixed, because there is *i, but will >make pr59470.c (the new one in the patch) broken, because there the >SSA_NAME is used as ARRAY_REF index and base is some VAR_DECL. >It guess it wouldn't be hard to make similar testcase even for the call >case, though it is unclear if it would be miscompiled or not. > > Jakub
--- gcc/tree-ssa-ter.c.jj 2013-12-10 08:52:13.000000000 +0100 +++ gcc/tree-ssa-ter.c 2013-12-12 10:43:26.177866960 +0100 @@ -554,6 +554,20 @@ mark_replaceable (temp_expr_table_p tab, } +/* Helper function for find_replaceable_in_bb. Called via walk_tree to + find a SSA_NAME DATA somewhere in *TP. */ + +static tree +find_ssa_name (tree *tp, int *walk_subtrees, void *data) +{ + tree var = (tree) data; + if (*tp == var) + return var; + else if (IS_TYPE_OR_DECL_P (*tp)) + *walk_subtrees = 0; + return NULL_TREE; +} + /* This function processes basic block BB, and looks for variables which can be replaced by their expressions. Results are stored in the table TAB. */ @@ -618,7 +632,42 @@ find_replaceable_in_bb (temp_expr_table_ && gimple_assign_single_p (def_stmt) && stmt_may_clobber_ref_p (stmt, gimple_assign_rhs1 (def_stmt))) - same_root_var = true; + { + if (is_gimple_call (stmt)) + { + /* For calls, it is not a problem if USE is among + call's arguments or say OBJ_TYPE_REF argument, + all those necessarily need to be evaluated before + the call that may clobber the memory. But if + LHS of the call refers to USE, expansion might + evaluate it after the call, prevent TER in that + case. */ + if (gimple_call_lhs (stmt) + && TREE_CODE (gimple_call_lhs (stmt)) != SSA_NAME + && walk_tree (gimple_call_lhs_ptr (stmt), + find_ssa_name, use, NULL)) + same_root_var = true; + } + else if (gimple_code (stmt) == GIMPLE_ASM) + { + /* For inline asm, allow TER of loads into input + arguments, but disallow TER for USEs that occur + somewhere in outputs. */ + unsigned int i; + for (i = 0; i < gimple_asm_noutputs (stmt); i++) + if (TREE_CODE (gimple_asm_output_op (stmt, i)) + != SSA_NAME + && walk_tree (gimple_asm_output_op_ptr (stmt, + i), + find_ssa_name, use, NULL)) + { + same_root_var = true; + break; + } + } + else + same_root_var = true; + } } /* Mark expression as replaceable unless stmt is volatile, or the --- gcc/testsuite/gcc.target/i386/pr59470.c.jj 2013-12-12 10:31:54.746517544 +0100 +++ gcc/testsuite/gcc.target/i386/pr59470.c 2013-12-12 10:32:42.045273313 +0100 @@ -0,0 +1,17 @@ +/* PR middle-end/58956 */ +/* PR middle-end/59470 */ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +int a, b, d[1024]; + +int +main () +{ + int c = a; + asm ("{movl $6, (%2); movl $1, %0|mov dword ptr [%2], 6; mov %0, 1}" + : "=r" (d[c]) : "rm" (b), "r" (&a) : "memory"); + if (d[0] != 1 || d[6] != 0) + __builtin_abort (); + return 0; +}