| Message ID | alpine.LSU.2.20.1702101310510.6076@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
On 02/10/2017 05:24 AM, Richard Biener wrote: > > It turns out the SSA var defs/uses generated by the gimplifier can break > apart in a way no longer satisfying the dominance requirement of SSA > uses vs. their defs by means of CFG construction adding abnormal edges > for stuff like setjmp (but also non-local gotos I guess). > > This would be quite costly to overcome in gimplification - one needs > to check whether a (part?) of an expression to be gimplified may > produce such edges and disable SSA name generation for them. With > the recursive nature of the gimplifier plus the general complexity > of GENERIC I can't see how to do this. ISTM there is no apriori way to know if any object is live across a call which would create these abnormal edges. How in the world did this work in the past? Sigh. Because the problem starts at gimplification, we can't even do something like pre-scan the block for problem calls -- we don't have a CFG. Can this only happen during gimplification of an expression where a sub-expression has a problematic call? How bad do you think things would blow up if we scanned for a call and avoided using SSA_NAMEs when there's a problematical call within the expression? Or is there no way structurally to do that within the gimplifier? Are setjmp/longjump the only problems here, or does EH tickle these issues as well (if so, then ISTM you'd need a different test). > > Thus the following patch "recovers" from the extra abnormal edges > by effectively treating SSA vars pre into-SSA as "non-SSA" and thus > doing PHI insertion for them when rewriting the function into SSA. > Implementation-wise the easiest thing was to re-write the affected > SSA vars out-of-SSA (replace them by a decl). If we can't do the right thing from the start, then "recovery" seems to be the only option. > > The out-of-SSA rewriting is placed in insert_phi_nodes because thats > the first point in time we have immediate uses present (otherwise > it could be done at any point after CFG construction). THe earlier the better. So as soon as we have a CFG & SSA we have to fixup. So location seems right. > > I'm not 100% happy with this but after trying for a day I can't come > up with a better solution - it has the chance of papering over > bogus gimplifications but OTOH that we only do this for functions > calling setjmp or having non-local labels would make those trigger > SSA verification in the other cases. > > Bootstrap and regtest running on x86_64-unknown-linux-gnu. > > Any comments / ideas? Anyone with good arguments that this is > even conceptually the correct thing to do? > > I'm bootstrapping this with the calls_setjmp/has_nonlocal_label > short-cutted with a 1 || as well as without (to increase testing > coverage). > > Thanks, > Richard. > > 2017-02-10 Richard Biener <rguenther@suse.de> > > PR middle-end/79432 > * tree-into-ssa.c (insert_phi_nodes): When the function can > have abnormal edges rewrite SSA names with broken use-def > dominance out of SSA and register them for PHI insertion. > > * gcc.dg/torture/pr79432.c: New testcase. It's gross. But unless we have a reasonable way to do this during gimplification, I don't see anything better. jeff
On Mon, 13 Feb 2017, Jeff Law wrote: > On 02/10/2017 05:24 AM, Richard Biener wrote: > > > > It turns out the SSA var defs/uses generated by the gimplifier can break > > apart in a way no longer satisfying the dominance requirement of SSA > > uses vs. their defs by means of CFG construction adding abnormal edges > > for stuff like setjmp (but also non-local gotos I guess). > > > > This would be quite costly to overcome in gimplification - one needs > > to check whether a (part?) of an expression to be gimplified may > > produce such edges and disable SSA name generation for them. With > > the recursive nature of the gimplifier plus the general complexity > > of GENERIC I can't see how to do this. > ISTM there is no apriori way to know if any object is live across a call which > would create these abnormal edges. How in the world did this work in the > past? It worked in the past because we didn't use SSA names for temporaries in the gimplifiers and regular decls just got effectively uninitialized uses across such edges (by into-SSA and PHI insertion, exactly how I fix this "after the fact") > Sigh. Because the problem starts at gimplification, we can't even do > something like pre-scan the block for problem calls -- we don't have a CFG. > > Can this only happen during gimplification of an expression where a > sub-expression has a problematic call? How bad do you think things would blow > up if we scanned for a call and avoided using SSA_NAMEs when there's a > problematical call within the expression? Or is there no way structurally to > do that within the gimplifier? Well, the only thing I could imagine is doing walk_tree on all GENERIC where it matters (all?). > Are setjmp/longjump the only problems here, or does EH tickle these issues as > well (if so, then ISTM you'd need a different test). The only issue is when CFG construction adds backedges which only happens for abnormals. Only backedges can make uses no longer dominate defs. > > > > Thus the following patch "recovers" from the extra abnormal edges > > by effectively treating SSA vars pre into-SSA as "non-SSA" and thus > > doing PHI insertion for them when rewriting the function into SSA. > > Implementation-wise the easiest thing was to re-write the affected > > SSA vars out-of-SSA (replace them by a decl). > If we can't do the right thing from the start, then "recovery" seems to be the > only option. I think it's a question of cost and maintainability. It for sure must be possible to fix this in the gimplifier but I think the costs are too high. > > > > > The out-of-SSA rewriting is placed in insert_phi_nodes because thats > > the first point in time we have immediate uses present (otherwise > > it could be done at any point after CFG construction). > THe earlier the better. So as soon as we have a CFG & SSA we have to fixup. > So location seems right. Yeah, if we'd have immediate uses at CFG construction then I'd fix it at the time we introduce those abnormal edges. For GCC 8 we might want to consider this (building SSA operands earlier). > > > > I'm not 100% happy with this but after trying for a day I can't come > > up with a better solution - it has the chance of papering over > > bogus gimplifications but OTOH that we only do this for functions > > calling setjmp or having non-local labels would make those trigger > > SSA verification in the other cases. > > > > Bootstrap and regtest running on x86_64-unknown-linux-gnu. > > > > Any comments / ideas? Anyone with good arguments that this is > > even conceptually the correct thing to do? > > > > I'm bootstrapping this with the calls_setjmp/has_nonlocal_label > > short-cutted with a 1 || as well as without (to increase testing > > coverage). > > > > Thanks, > > Richard. > > > > 2017-02-10 Richard Biener <rguenther@suse.de> > > > > PR middle-end/79432 > > * tree-into-ssa.c (insert_phi_nodes): When the function can > > have abnormal edges rewrite SSA names with broken use-def > > dominance out of SSA and register them for PHI insertion. > > > > * gcc.dg/torture/pr79432.c: New testcase. > It's gross. But unless we have a reasonable way to do this during > gimplification, I don't see anything better. Ok. I've applied the patch for now. If anybody comes up with a cheap enough idea to cover these in the gimplifier we can reconsider. Richard. > jeff > >
Index: gcc/tree-into-ssa.c =================================================================== --- gcc/tree-into-ssa.c (revision 245323) +++ gcc/tree-into-ssa.c (working copy) @@ -1066,6 +1066,59 @@ insert_phi_nodes (bitmap_head *dfs) timevar_push (TV_TREE_INSERT_PHI_NODES); + /* When the gimplifier introduces SSA names it cannot easily avoid + situations where abnormal edges added by CFG construction break + the use-def dominance requirement. For this case rewrite SSA + names with broken use-def dominance out-of-SSA and register them + for PHI insertion. We only need to do this if abnormal edges + can appear in the function. */ + tree name; + if (cfun->calls_setjmp + || cfun->has_nonlocal_label) + FOR_EACH_SSA_NAME (i, name, cfun) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (name); + if (SSA_NAME_IS_DEFAULT_DEF (name)) + continue; + + basic_block def_bb = gimple_bb (def_stmt); + imm_use_iterator it; + gimple *use_stmt; + bool need_phis = false; + FOR_EACH_IMM_USE_STMT (use_stmt, it, name) + { + basic_block use_bb = gimple_bb (use_stmt); + if (use_bb != def_bb + && ! dominated_by_p (CDI_DOMINATORS, use_bb, def_bb)) + need_phis = true; + } + if (need_phis) + { + tree var = create_tmp_reg (TREE_TYPE (name)); + use_operand_p use_p; + FOR_EACH_IMM_USE_STMT (use_stmt, it, name) + { + basic_block use_bb = gimple_bb (use_stmt); + FOR_EACH_IMM_USE_ON_STMT (use_p, it) + SET_USE (use_p, var); + update_stmt (use_stmt); + set_livein_block (var, use_bb); + set_rewrite_uses (use_stmt, true); + bitmap_set_bit (interesting_blocks, use_bb->index); + } + def_operand_p def_p; + ssa_op_iter dit; + FOR_EACH_SSA_DEF_OPERAND (def_p, def_stmt, dit, SSA_OP_DEF) + if (DEF_FROM_PTR (def_p) == name) + SET_DEF (def_p, var); + update_stmt (def_stmt); + set_def_block (var, def_bb, false); + set_register_defs (def_stmt, true); + bitmap_set_bit (interesting_blocks, def_bb->index); + release_ssa_name (name); + } + } + auto_vec<var_info *> vars (var_infos->elements ()); FOR_EACH_HASH_TABLE_ELEMENT (*var_infos, info, var_info_p, hi) if (info->info.need_phi_state != NEED_PHI_STATE_NO) Index: gcc/testsuite/gcc.dg/torture/pr79432.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr79432.c (nonexistent) +++ gcc/testsuite/gcc.dg/torture/pr79432.c (working copy) @@ -0,0 +1,20 @@ +/* { dg-do compile } */ + +int fn1 (void); +int __attribute__((returns_twice)) vfork (void); + +void fn2 () +{ + int a; + a = fn1() + 2 + (vfork() + 1 + vfork()); +} +void fn3 () +{ + int a; + a = fn1() + 1 + vfork(); +} +void fn4 () +{ + int a; + a = fn1() + vfork(); +}