| Message ID | alpine.LSU.2.21.1811201323280.5354@wotan.suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR37916 (unnecessary spilling) | expand |
On Tue, 20 Nov 2018, Michael Matz wrote: > > I'm wondering about testcase: should I add an x86-64 specific that tests > for no stack accesses, or would that be too constraining in the future? > > Regstrapped on x86-64-linux, no regressions. Okay for trunk? By the way, this patch helps x86-64 on PR 84681. Some unnecessary spills remain in the loop, but not as many as without the patch. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84681 Alexander
On 11/20/18 6:42 AM, Michael Matz wrote: > Hi, > > this bug report is about cris generating worse code since tree-ssa. The > effect is also visible on x86-64. The symptom is that the work horse of > adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not, > and those spills go away with -fno-tree-reassoc. > > The underlying reason for the spills is register pressure, which could > either be rectified by the pressure aware scheduling (which cris doesn't > have), or by simply not generating high pressure code to begin with. In > this case it's TER which ultimately causes the register pressure to > increase, and there are many plans in people minds how to fix this (make > TER regpressure aware, do some regpressure scheduling on gimple, or even > more ambitious things), but this patch doesn't tackle this. Instead it > makes reassoc not generate the situation which causes TER to run wild. > > TER increasing register pressure is a long standing problem and so it has > some heuristics to avoid that. One wobbly heuristic is that it doesn't > TER expressions together that have the same base variable as their LHSs. > But reassoc generates only anonymous SSA names for its temporary > subexpressions, so that TER heuristic doesn't apply. In this testcase > it's even the case that reassoc doesn't really change much code (one > addition moves from the end to the beginning of the inner loop), so that > whole rewriting is even pointless. > > In any case, let's use copy_ssa_name instead of make_ssa_name, when we > have an obvious LHS; that leads to TER making the same decisions with and > without -fno-tree-reassoc, leading to the same register pressure and no > spills. > > On x86-64 the effect is: > before patch: 48 bytes stackframe, 24 stack > accesses (most of them in the loops), 576 bytes codesize; > after patch: no stack frame, no stack accesses, 438 bytes codesize > > On cris: > before patch: 64 bytes stack frame, 27 stack access in loops, size of .s > 145 lines > after patch: 20 bytes stack frame (as it uses callee saved regs, which > is also complained about in the bug report), but no stack accesses > in loops, size of .s: 125 lines > > I'm wondering about testcase: should I add an x86-64 specific that tests > for no stack accesses, or would that be too constraining in the future? I think that'd be a fine test. Yea it's constraining, but isn't that the point, to make sure we never generate something dumb for this code. If we get to a point where we have to have a stack reference we can re-evaluate the test. > > Regstrapped on x86-64-linux, no regressions. Okay for trunk? > > > Ciao, > Michael. > > PR tree-optimization/37916 > * tree-ssa-reassoc.c (make_new_ssa_for_def): Use copy_ssa_name. > (rewrite_expr_tree, linearize_expr, negate_value, > repropagate_negates, attempt_builtin_copysign, > reassociate_bb): Likewise. I wonder if this would help 21182. I don't think anyone ever looked to see if reassoc was involved in the issues in that BZ. OK for the trunk. jeff
On Wed, Nov 21, 2018 at 1:12 AM Jeff Law <law@redhat.com> wrote: > > On 11/20/18 6:42 AM, Michael Matz wrote: > > Hi, > > > > this bug report is about cris generating worse code since tree-ssa. The > > effect is also visible on x86-64. The symptom is that the work horse of > > adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not, > > and those spills go away with -fno-tree-reassoc. > > > > The underlying reason for the spills is register pressure, which could > > either be rectified by the pressure aware scheduling (which cris doesn't > > have), or by simply not generating high pressure code to begin with. In > > this case it's TER which ultimately causes the register pressure to > > increase, and there are many plans in people minds how to fix this (make > > TER regpressure aware, do some regpressure scheduling on gimple, or even > > more ambitious things), but this patch doesn't tackle this. Instead it > > makes reassoc not generate the situation which causes TER to run wild. > > > > TER increasing register pressure is a long standing problem and so it has > > some heuristics to avoid that. One wobbly heuristic is that it doesn't > > TER expressions together that have the same base variable as their LHSs. > > But reassoc generates only anonymous SSA names for its temporary > > subexpressions, so that TER heuristic doesn't apply. In this testcase > > it's even the case that reassoc doesn't really change much code (one > > addition moves from the end to the beginning of the inner loop), so that > > whole rewriting is even pointless. > > > > In any case, let's use copy_ssa_name instead of make_ssa_name, when we > > have an obvious LHS; that leads to TER making the same decisions with and > > without -fno-tree-reassoc, leading to the same register pressure and no > > spills. I don't think this is OK. Take one example, in rewrite_expr_tree for the final recursion case we replace a_1 = _2 + _3; with sth else, like _4 = _5 + 1; so we compute a new value that may not have been computed before and certainly not into the user variable a. If you change this to instead create a_4 = _5 + 1; then this leads to wrong debug info showing a value for 'a' that never existed. You can observe this with unsigned int __attribute__((noinline,noipa)) foo (unsigned int a, unsigned int b, unsigned int c, unsigned int d) { a = a + 1; a = a + b; a = a + c; a = a + d; a = a + 3; return a; } int main() { volatile unsigned x = foo (1, 3, 5, 7); return 0; } when you build this with -O -g -fno-var-tracking-assignments (VTA seems to hide the issue, probably due to not enough instructions in the end) you observe (gdb) s foo (a=1, b=3, c=5, d=7) at t.c:6 6 a = a + c; (gdb) disassemble Dump of assembler code for function foo: => 0x00000000004004b2 <+0>: lea 0x4(%rdx,%rcx,1),%eax 0x00000000004004b6 <+4>: add %esi,%eax 0x00000000004004b8 <+6>: add %edi,%eax 0x00000000004004ba <+8>: retq End of assembler dump. (gdb) p a $1 = 1 (gdb) si 7 a = a + d; (gdb) p a $2 = 16 (gdb) si 9 return a; (gdb) p a $3 = 19 (gdb) si 0x00000000004004ba 9 return a; (gdb) p a $4 = <optimized out> (gdb) p $eax $5 = 20 (gdb) quit printing values for a that never should occur. The sequence of "allowed" values is 1, 2, 5, 10, 17, 20. Both 16 and 19 should never be printed. It's quite obvious if you look at the reassoc result which is a_11 = d_7(D) + 4; a_12 = a_11 + c_5(D); a_13 = a_12 + b_3(D); a_9 = a_13 + a_1(D); return a_9; and the fact that var-tracking looks at REG_DECL for debug info location generation. With VTA we get # DEBUG BEGIN_STMT # DEBUG D#3 => a_1(D) + 1 # DEBUG a => D#3 # DEBUG BEGIN_STMT a_11 = d_7(D) + 4; # DEBUG D#2 => D#3 + b_3(D) # DEBUG a => D#2 # DEBUG BEGIN_STMT a_12 = a_11 + c_5(D); # DEBUG D#1 => D#2 + c_5(D) # DEBUG a => D#1 # DEBUG BEGIN_STMT a_13 = a_12 + b_3(D); # DEBUG a => D#1 + d_7(D) # DEBUG BEGIN_STMT a_9 = a_13 + a_1(D); # DEBUG a => a_9 # DEBUG BEGIN_STMT return a_9; but DEBUG stmts in itself do not make the code valid from a debug perspective. Note that the reassociated stmts can become DEBUG stmts itself if they are later DCEd. So your patch has to be much more careful to never change the LHS of stmts that are adjusted (which I think reassoc already does). Richard. > > On x86-64 the effect is: > > before patch: 48 bytes stackframe, 24 stack > > accesses (most of them in the loops), 576 bytes codesize; > > after patch: no stack frame, no stack accesses, 438 bytes codesize > > > > On cris: > > before patch: 64 bytes stack frame, 27 stack access in loops, size of .s > > 145 lines > > after patch: 20 bytes stack frame (as it uses callee saved regs, which > > is also complained about in the bug report), but no stack accesses > > in loops, size of .s: 125 lines > > > > I'm wondering about testcase: should I add an x86-64 specific that tests > > for no stack accesses, or would that be too constraining in the future? > I think that'd be a fine test. Yea it's constraining, but isn't that > the point, to make sure we never generate something dumb for this code. > > If we get to a point where we have to have a stack reference we can > re-evaluate the test. > > > > > Regstrapped on x86-64-linux, no regressions. Okay for trunk? > > > > > > Ciao, > > Michael. > > > > PR tree-optimization/37916 > > * tree-ssa-reassoc.c (make_new_ssa_for_def): Use copy_ssa_name. > > (rewrite_expr_tree, linearize_expr, negate_value, > > repropagate_negates, attempt_builtin_copysign, > > reassociate_bb): Likewise. > I wonder if this would help 21182. I don't think anyone ever looked to > see if reassoc was involved in the issues in that BZ. > > OK for the trunk. > > jeff
On 11/21/18 7:13 AM, Richard Biener wrote: > On Wed, Nov 21, 2018 at 1:12 AM Jeff Law <law@redhat.com> wrote: >> >> On 11/20/18 6:42 AM, Michael Matz wrote: >>> Hi, >>> >>> this bug report is about cris generating worse code since tree-ssa. The >>> effect is also visible on x86-64. The symptom is that the work horse of >>> adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not, >>> and those spills go away with -fno-tree-reassoc. >>> >>> The underlying reason for the spills is register pressure, which could >>> either be rectified by the pressure aware scheduling (which cris doesn't >>> have), or by simply not generating high pressure code to begin with. In >>> this case it's TER which ultimately causes the register pressure to >>> increase, and there are many plans in people minds how to fix this (make >>> TER regpressure aware, do some regpressure scheduling on gimple, or even >>> more ambitious things), but this patch doesn't tackle this. Instead it >>> makes reassoc not generate the situation which causes TER to run wild. >>> >>> TER increasing register pressure is a long standing problem and so it has >>> some heuristics to avoid that. One wobbly heuristic is that it doesn't >>> TER expressions together that have the same base variable as their LHSs. >>> But reassoc generates only anonymous SSA names for its temporary >>> subexpressions, so that TER heuristic doesn't apply. In this testcase >>> it's even the case that reassoc doesn't really change much code (one >>> addition moves from the end to the beginning of the inner loop), so that >>> whole rewriting is even pointless. >>> >>> In any case, let's use copy_ssa_name instead of make_ssa_name, when we >>> have an obvious LHS; that leads to TER making the same decisions with and >>> without -fno-tree-reassoc, leading to the same register pressure and no >>> spills. > > I don't think this is OK. Take one example, in rewrite_expr_tree for the final > recursion case we replace > > a_1 = _2 + _3; > > with sth else, like > > _4 = _5 + 1; > > so we compute a new value that may not have been computed before and > certainly not into the user variable a. If you change this to instead create > > a_4 = _5 + 1; > > then this leads to wrong debug info showing a value for 'a' that never existed. > You can observe this with But isn't the point to use an underlying SSA_NAME_VAR when we have one that should be appropriate? Are we just being too aggressive with using copy_ssa_name? jeff
On Wed, Nov 21, 2018 at 3:16 PM Jeff Law <law@redhat.com> wrote: > > On 11/21/18 7:13 AM, Richard Biener wrote: > > On Wed, Nov 21, 2018 at 1:12 AM Jeff Law <law@redhat.com> wrote: > >> > >> On 11/20/18 6:42 AM, Michael Matz wrote: > >>> Hi, > >>> > >>> this bug report is about cris generating worse code since tree-ssa. The > >>> effect is also visible on x86-64. The symptom is that the work horse of > >>> adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not, > >>> and those spills go away with -fno-tree-reassoc. > >>> > >>> The underlying reason for the spills is register pressure, which could > >>> either be rectified by the pressure aware scheduling (which cris doesn't > >>> have), or by simply not generating high pressure code to begin with. In > >>> this case it's TER which ultimately causes the register pressure to > >>> increase, and there are many plans in people minds how to fix this (make > >>> TER regpressure aware, do some regpressure scheduling on gimple, or even > >>> more ambitious things), but this patch doesn't tackle this. Instead it > >>> makes reassoc not generate the situation which causes TER to run wild. > >>> > >>> TER increasing register pressure is a long standing problem and so it has > >>> some heuristics to avoid that. One wobbly heuristic is that it doesn't > >>> TER expressions together that have the same base variable as their LHSs. > >>> But reassoc generates only anonymous SSA names for its temporary > >>> subexpressions, so that TER heuristic doesn't apply. In this testcase > >>> it's even the case that reassoc doesn't really change much code (one > >>> addition moves from the end to the beginning of the inner loop), so that > >>> whole rewriting is even pointless. > >>> > >>> In any case, let's use copy_ssa_name instead of make_ssa_name, when we > >>> have an obvious LHS; that leads to TER making the same decisions with and > >>> without -fno-tree-reassoc, leading to the same register pressure and no > >>> spills. > > > > I don't think this is OK. Take one example, in rewrite_expr_tree for the final > > recursion case we replace > > > > a_1 = _2 + _3; > > > > with sth else, like > > > > _4 = _5 + 1; > > > > so we compute a new value that may not have been computed before and > > certainly not into the user variable a. If you change this to instead create > > > > a_4 = _5 + 1; > > > > then this leads to wrong debug info showing a value for 'a' that never existed. > > You can observe this with > But isn't the point to use an underlying SSA_NAME_VAR when we have one > that should be appropriate? Are we just being too aggressive with using > copy_ssa_name? Well, sure - there _might_ be places that could use copy_ssa_name in reassoc but I doubt so. Usually code-generation should not use copy_ssa_name given the aforementioned issues. Richard. > > jeff >
Hi, On Wed, 21 Nov 2018, Richard Biener wrote: > then this leads to wrong debug info showing a value for 'a' that never > existed. var-tracking was specifically created so that the base-vars of SSA names aren't necessary for debug info anymore ... > when you build this with -O -g -fno-var-tracking-assignments (VTA seems > to hide the issue, probably due to not enough instructions in the end) ... which makes this an non-test. Sure, if you cripple debug info generation you get ... crippled debug info. > and the fact that var-tracking looks at REG_DECL for > debug info location generation. With VTA we get > > # DEBUG BEGIN_STMT > # DEBUG D#3 => a_1(D) + 1 > # DEBUG a => D#3 > # DEBUG BEGIN_STMT > a_11 = d_7(D) + 4; > # DEBUG D#2 => D#3 + b_3(D) > # DEBUG a => D#2 > # DEBUG BEGIN_STMT > a_12 = a_11 + c_5(D); > # DEBUG D#1 => D#2 + c_5(D) > # DEBUG a => D#1 > # DEBUG BEGIN_STMT > a_13 = a_12 + b_3(D); > # DEBUG a => D#1 + d_7(D) > # DEBUG BEGIN_STMT > a_9 = a_13 + a_1(D); > # DEBUG a => a_9 This seems to be exactly correct and reflecting the original code, i.e. VTA works as designed even with "invented" base variables. > So your patch has to be much more careful to never change the LHS of > stmts that are adjusted (which I think reassoc already does). I can't do much more in reassoc than what I implemented, it's constrained to cases where we have a known LHS. If you think that's still too much then the problem needs to remain unfixed in reassoc, and it would need TER changes. An easy way would be to disable TERin also of SSA names having no base var (essentially on the ground that it's the same base var then), but that probably disables TER too much, though I haven't tried. In addition: I'd always happily trade less correct debug info (where the impact of the incorrectness is fairly trivial) with no stack accesses in inner loops; i.e. a tradeoff. Ciao, Michael.
On Thu, Nov 22, 2018 at 01:30:03PM +0000, Michael Matz wrote: > On Wed, 21 Nov 2018, Richard Biener wrote: > > > then this leads to wrong debug info showing a value for 'a' that never > > existed. > > var-tracking was specifically created so that the base-vars of SSA names > aren't necessary for debug info anymore ... Not to my knowledge. The original var-tracking used that information almost exclusively (whatever ends up in REG_EXPR/MEM_EXPR and that is based on the base vars of SSA_NAMEs). VTA doesn't need that for vars for which there are debug stmts, but otherwise it is still used. Jakub
On Thu, Nov 22, 2018 at 2:34 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Nov 22, 2018 at 01:30:03PM +0000, Michael Matz wrote: > > On Wed, 21 Nov 2018, Richard Biener wrote: > > > > > then this leads to wrong debug info showing a value for 'a' that never > > > existed. > > > > var-tracking was specifically created so that the base-vars of SSA names > > aren't necessary for debug info anymore ... > > Not to my knowledge. The original var-tracking used that information > almost exclusively (whatever ends up in REG_EXPR/MEM_EXPR and that is based > on the base vars of SSA_NAMEs). > VTA doesn't need that for vars for which there are debug stmts, but > otherwise it is still used. Also it still uses it even for vars for which there are debug stmts, no? We just happen to be lucky for the too simple testcase that only the correct values survive for the very few $pc addresses? Richard. > Jakub
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index 971d926e7895..339c3d4e447f 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -1182,7 +1182,7 @@ make_new_ssa_for_def (gimple *stmt, enum tree_code opcode, tree op) tree new_lhs, new_debug_lhs = NULL_TREE; tree lhs = gimple_get_lhs (stmt); - new_lhs = make_ssa_name (TREE_TYPE (lhs)); + new_lhs = copy_ssa_name (lhs); gimple_set_lhs (stmt, new_lhs); /* Also need to update GIMPLE_DEBUGs. */ @@ -4512,7 +4512,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex, { gimple *insert_point = find_insert_point (stmt, oe1->op, oe2->op); - lhs = make_ssa_name (TREE_TYPE (lhs)); + lhs = copy_ssa_name (lhs); stmt = gimple_build_assign (lhs, gimple_assign_rhs_code (stmt), oe1->op, oe2->op); @@ -4583,7 +4583,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex, unsigned int uid = gimple_uid (stmt); gimple *insert_point = find_insert_point (stmt, new_rhs1, oe->op); - lhs = make_ssa_name (TREE_TYPE (lhs)); + lhs = copy_ssa_name (lhs); stmt = gimple_build_assign (lhs, gimple_assign_rhs_code (stmt), new_rhs1, oe->op); gimple_set_uid (stmt, uid); @@ -4820,7 +4820,7 @@ linearize_expr (gimple *stmt) gsi = gsi_for_stmt (stmt); gimple_assign_set_rhs2 (stmt, gimple_assign_rhs1 (binrhs)); - binrhs = gimple_build_assign (make_ssa_name (TREE_TYPE (lhs)), + binrhs = gimple_build_assign (copy_ssa_name (lhs), gimple_assign_rhs_code (binrhs), gimple_assign_lhs (binlhs), gimple_assign_rhs2 (binrhs)); @@ -4909,7 +4909,7 @@ negate_value (tree tonegate, gimple_stmt_iterator *gsip) rhs2 = negate_value (rhs2, &gsi); gsi = gsi_for_stmt (negatedefstmt); - lhs = make_ssa_name (TREE_TYPE (lhs)); + lhs = copy_ssa_name (lhs); gimple_set_visited (negatedefstmt, true); g = gimple_build_assign (lhs, PLUS_EXPR, rhs1, rhs2); gimple_set_uid (g, gimple_uid (negatedefstmt)); @@ -5245,7 +5245,7 @@ repropagate_negates (void) tree b = gimple_assign_rhs2 (user); gimple_stmt_iterator gsi = gsi_for_stmt (feed); gimple_stmt_iterator gsi2 = gsi_for_stmt (user); - tree x = make_ssa_name (TREE_TYPE (gimple_assign_lhs (feed))); + tree x = copy_ssa_name (gimple_assign_lhs (feed)); gimple *g = gimple_build_assign (x, PLUS_EXPR, a, b); gsi_insert_before (&gsi2, g, GSI_SAME_STMT); gimple_assign_set_rhs_with_ops (&gsi2, NEGATE_EXPR, x); @@ -5737,7 +5737,7 @@ attempt_builtin_copysign (vec<operand_entry *> *ops) else new_call = gimple_build_call (gimple_call_fndecl (old_call), 2, mul, arg1); - tree lhs = make_ssa_name (type); + tree lhs = copy_ssa_name (oe->op); gimple_call_set_lhs (new_call, lhs); gimple_set_location (new_call, gimple_location (old_call)); @@ -5748,7 +5748,7 @@ attempt_builtin_copysign (vec<operand_entry *> *ops) /* Handle the CST1 < 0 case by negating the result. */ if (cst1_neg) { - tree negrhs = make_ssa_name (TREE_TYPE (lhs)); + tree negrhs = copy_ssa_name (lhs); gimple *negate_stmt = gimple_build_assign (negrhs, NEGATE_EXPR, lhs); insert_stmt_after (negate_stmt, new_call); @@ -6052,7 +6052,7 @@ reassociate_bb (basic_block bb) if (negate_result) { stmt = SSA_NAME_DEF_STMT (lhs); - tree tmp = make_ssa_name (TREE_TYPE (lhs)); + tree tmp = copy_ssa_name (lhs); gimple_set_lhs (stmt, tmp); if (lhs != new_lhs) tmp = new_lhs;
Hi, this bug report is about cris generating worse code since tree-ssa. The effect is also visible on x86-64. The symptom is that the work horse of adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not, and those spills go away with -fno-tree-reassoc. The underlying reason for the spills is register pressure, which could either be rectified by the pressure aware scheduling (which cris doesn't have), or by simply not generating high pressure code to begin with. In this case it's TER which ultimately causes the register pressure to increase, and there are many plans in people minds how to fix this (make TER regpressure aware, do some regpressure scheduling on gimple, or even more ambitious things), but this patch doesn't tackle this. Instead it makes reassoc not generate the situation which causes TER to run wild. TER increasing register pressure is a long standing problem and so it has some heuristics to avoid that. One wobbly heuristic is that it doesn't TER expressions together that have the same base variable as their LHSs. But reassoc generates only anonymous SSA names for its temporary subexpressions, so that TER heuristic doesn't apply. In this testcase it's even the case that reassoc doesn't really change much code (one addition moves from the end to the beginning of the inner loop), so that whole rewriting is even pointless. In any case, let's use copy_ssa_name instead of make_ssa_name, when we have an obvious LHS; that leads to TER making the same decisions with and without -fno-tree-reassoc, leading to the same register pressure and no spills. On x86-64 the effect is: before patch: 48 bytes stackframe, 24 stack accesses (most of them in the loops), 576 bytes codesize; after patch: no stack frame, no stack accesses, 438 bytes codesize On cris: before patch: 64 bytes stack frame, 27 stack access in loops, size of .s 145 lines after patch: 20 bytes stack frame (as it uses callee saved regs, which is also complained about in the bug report), but no stack accesses in loops, size of .s: 125 lines I'm wondering about testcase: should I add an x86-64 specific that tests for no stack accesses, or would that be too constraining in the future? Regstrapped on x86-64-linux, no regressions. Okay for trunk? Ciao, Michael. PR tree-optimization/37916 * tree-ssa-reassoc.c (make_new_ssa_for_def): Use copy_ssa_name. (rewrite_expr_tree, linearize_expr, negate_value, repropagate_negates, attempt_builtin_copysign, reassociate_bb): Likewise.