| Message ID | 1538996.TcTA0zy8bB@polaris |
|---|---|
| State | New |
| Headers | show |
On Fri, May 1, 2015 at 8:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> OK, how aggressive then? We could as well do the substitution for all >> copies: >> >> /* For EXPAND_INITIALIZER try harder to get something simpler. >> Otherwise, substitute copies on the RHS, this can propagate >> constants at -O0 and thus simplify arithmetic operations. */ >> if (g == NULL >> && !SSA_NAME_IS_DEFAULT_DEF (exp) >> && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp))) >> && (modifier == EXPAND_INITIALIZER >> >> || (modifier != EXPAND_WRITE >> >> && gimple_assign_copy_p (SSA_NAME_DEF_STMT (exp)))) >> && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp))) >> g = SSA_NAME_DEF_STMT (exp); > > This doesn't work (this generates wrong code because this creates overlapping > live ranges for SSA_NAMEs with the same base variable). Here's the latest > working version, all the predicates and accessors used are inlined. Hum, the fact that your earlier version created wrong code (get_gimple_for_ssa_name already returned false here) points at some issues with EXPAND_INITIALIZER as well, no...? That said, the path you add is certainly safe (though maybe we want to change get_gimple_for_ssa_name to return tcc_constant single-use defs even if TER is disabled (thus at -O0 - and only at -O0, otherwise it shouldn't happen). That would cover more cases of get_gimple_for_ssa_name uses (I can see optimize_bitfield_expansion for example...) So, your patch is ok for trunk unless you want to explore the get_gimple_for_ssa_name improvement suggestion. I also wonder about EXPAND_INITIALIZER creating overlapping life-ranges (or moving loads across stores). Thanks, Richard. > Tested on x86_64-suse-linux, OK for the mainline? > > > 2015-05-01 Eric Botcazou <ebotcazou@adacore.com> > > * expr.c (expand_expr_real_1) <SSA_NAME>: Try to substitute constants > on the RHS of expressions. > * gimple-expr.h (is_gimple_constant): Reorder. > > > -- > Eric Botcazou
> Hum, the fact that your earlier version created wrong code > (get_gimple_for_ssa_name > already returned false here) points at some issues with > EXPAND_INITIALIZER as well, no...? Theoritically yes but, in practice, EXPAND_INITIALIZER is used in varasm.c and for debugging stuff only, so I don't think that's a real concern. > That said, the path you add is certainly safe (though maybe we want to > change get_gimple_for_ssa_name to return tcc_constant single-use defs even > if TER is disabled > (thus at -O0 - and only at -O0, otherwise it shouldn't happen). That > would cover > more cases of get_gimple_for_ssa_name uses (I can see > optimize_bitfield_expansion > for example...) optimize_bitfield_assignment_op is only interested in loads from bitfields though. The get_gimple_for_ssa_name route would be interesting to bypass the stmt_is_replaceable_p test, i.e. to bypass the single-use test, but this could be counter-productive at -O0 so I'm not sure it's worth the trouble.
> 2015-05-01 Eric Botcazou <ebotcazou@adacore.com> > > * expr.c (expand_expr_real_1) <SSA_NAME>: Try to substitute constants > on the RHS of expressions. > * gimple-expr.h (is_gimple_constant): Reorder. Bummer. This breaks C++ debugging: +FAIL: gdb.cp/class2.exp: print alpha at marker return 0 +FAIL: gdb.cp/class2.exp: print beta at marker return 0 +FAIL: gdb.cp/class2.exp: print * aap at marker return 0 +FAIL: gdb.cp/class2.exp: print * bbp at marker return 0 +FAIL: gdb.cp/class2.exp: print * abp at marker return 0, s-p-o off +FAIL: gdb.cp/class2.exp: print * (B *) abp at marker return 0 +FAIL: gdb.cp/class2.exp: p acp +FAIL: gdb.cp/class2.exp: p acp->c1 +FAIL: gdb.cp/class2.exp: p acp->c2 because C++ is apparently relying on the assignment to the anonymous return object to preserve the debug info attached to a return statement. Would you be OK with a slight variation of your earlier idea, i.e. calling fold_stmt with a specific valueizer from fold_marked_statements instead of the implicit no_follow_ssa_edges in the inliner? Something like: tree follow_anonymous_single_use_edges (tree val) { if (TREE_CODE (val) == SSA_NAME && (!SSA_NAME_VAR (val) || DECL_IGNORED_P (SSA_NAME_VAR (var))) && has_single_use (val)) return val return NULL_TREE; }
On May 4, 2015 11:38:42 PM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote: >> 2015-05-01 Eric Botcazou <ebotcazou@adacore.com> >> >> * expr.c (expand_expr_real_1) <SSA_NAME>: Try to substitute >constants >> on the RHS of expressions. >> * gimple-expr.h (is_gimple_constant): Reorder. > >Bummer. This breaks C++ debugging: > >+FAIL: gdb.cp/class2.exp: print alpha at marker return 0 >+FAIL: gdb.cp/class2.exp: print beta at marker return 0 >+FAIL: gdb.cp/class2.exp: print * aap at marker return 0 >+FAIL: gdb.cp/class2.exp: print * bbp at marker return 0 >+FAIL: gdb.cp/class2.exp: print * abp at marker return 0, s-p-o off >+FAIL: gdb.cp/class2.exp: print * (B *) abp at marker return 0 >+FAIL: gdb.cp/class2.exp: p acp >+FAIL: gdb.cp/class2.exp: p acp->c1 >+FAIL: gdb.cp/class2.exp: p acp->c2 > >because C++ is apparently relying on the assignment to the anonymous >return >object to preserve the debug info attached to a return statement. > >Would you be OK with a slight variation of your earlier idea, i.e. >calling >fold_stmt with a specific valueizer from fold_marked_statements instead >of the >implicit no_follow_ssa_edges in the inliner? Something like: > >tree >follow_anonymous_single_use_edges (tree val) >{ > if (TREE_CODE (val) == SSA_NAME > && (!SSA_NAME_VAR (val) || DECL_IGNORED_P (SSA_NAME_VAR (var))) > && has_single_use (val)) > return val > return NULL_TREE; >} Yes, that works for me as well. Richard.
Index: expr.c =================================================================== --- expr.c (revision 222673) +++ expr.c (working copy) @@ -9511,11 +9511,17 @@ expand_expr_real_1 (tree exp, rtx target } g = get_gimple_for_ssa_name (exp); - /* For EXPAND_INITIALIZER try harder to get something simpler. */ + /* For EXPAND_INITIALIZER try harder to get something simpler. + Otherwise, substitute constants on the RHS, this can make + it possible to simplify arithmetic operations at -O0. */ if (g == NULL - && modifier == EXPAND_INITIALIZER && !SSA_NAME_IS_DEFAULT_DEF (exp) && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp))) + && (modifier == EXPAND_INITIALIZER + || (modifier != EXPAND_WRITE + && gimple_assign_single_p (SSA_NAME_DEF_STMT (exp)) + && is_gimple_constant + (gimple_assign_rhs1 (SSA_NAME_DEF_STMT (exp))))) && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp))) g = SSA_NAME_DEF_STMT (exp); if (g) Index: gimple-expr.h =================================================================== --- gimple-expr.h (revision 222673) +++ gimple-expr.h (working copy) @@ -136,9 +136,9 @@ is_gimple_constant (const_tree t) case INTEGER_CST: case REAL_CST: case FIXED_CST: - case STRING_CST: case COMPLEX_CST: case VECTOR_CST: + case STRING_CST: return true; default: