| Message ID | 87611owqn6.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 30, 2015 at 12:18 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > It's fairly easy to update the virtual ops when the call has no EH edges, > which should be cheaper than mark_virtual_operands_for_renaming. > > Tested on x86_64-linux-gnu, arm-linux-gnueabi and aarch64-linux-gnu. > OK to install? Well. I think this can be easily improved to handle the EH edge case by not replacing the virtual uses in the EH region. Btw, did you verify the pass does things correctly when facing an EH throwing situation? It seems all math builtins are marked as NOTHROW regardless of -fnon-call-exceptions ... Richard. > Thanks, > Richard > > > gcc/ > * tree-call-cdce.c (join_vdefs): New function. > (shrink_wrap_one_built_in_call): Use it when the call does not > need to end a block. Call mark_virtual_operands_for_renaming > otherwise. > (pass_call_cdce::execute): Don't call > mark_virtual_operands_for_renaming here. > > diff --git a/gcc/tree-call-cdce.c b/gcc/tree-call-cdce.c > index 72828dd..dcaa974 100644 > --- a/gcc/tree-call-cdce.c > +++ b/gcc/tree-call-cdce.c > @@ -708,6 +708,23 @@ gen_shrink_wrap_conditions (gcall *bi_call, vec<gimple *> conds, > /* Probability of the branch (to the call) is taken. */ > #define ERR_PROB 0.01 > > +/* Replace BI_CALL's vdef with a phi that uses BI_CALL's definition > + when WITH_CALL is taken and the previous definition when WITHOUT_CALL > + is taken. JOIN_BB is the target of both edges. */ > + > +static void > +join_vdefs (gcall *bi_call, edge with_call, edge without_call, > + basic_block join_bb) > +{ > + tree old_vuse = gimple_vuse (bi_call); > + tree old_vdef = gimple_vdef (bi_call); > + tree new_vdef = copy_ssa_name (old_vuse); > + gphi *phi = create_phi_node (new_vdef, join_bb); > + replace_uses_by (old_vdef, new_vdef); > + add_phi_arg (phi, old_vuse, without_call, UNKNOWN_LOCATION); > + add_phi_arg (phi, old_vdef, with_call, UNKNOWN_LOCATION); > +} > + > /* The function to shrink wrap a partially dead builtin call > whose return value is not used anywhere, but has to be kept > live due to potential error condition. Returns true if the > @@ -764,7 +781,8 @@ shrink_wrap_one_built_in_call (gcall *bi_call) > bi_call_bb = gimple_bb (bi_call); > > /* Now find the join target bb -- split bi_call_bb if needed. */ > - if (stmt_ends_bb_p (bi_call)) > + bool call_must_end_bb_p = stmt_ends_bb_p (bi_call); > + if (call_must_end_bb_p) > { > /* If the call must be the last in the bb, don't split the block, > it could e.g. have EH edges. */ > @@ -822,6 +840,12 @@ shrink_wrap_one_built_in_call (gcall *bi_call) > join_tgt_in_edge_fall_thru->count = > guard_bb->count - bi_call_in_edge0->count; > > + if (call_must_end_bb_p) > + mark_virtual_operands_for_renaming (cfun); > + else > + join_vdefs (bi_call, join_tgt_in_edge_from_call, > + join_tgt_in_edge_fall_thru, join_tgt_bb); > + > /* Code generation for the rest of the conditions */ > basic_block prev_guard_bb = NULL; > while (nconds > 0) > @@ -978,9 +1002,6 @@ pass_call_cdce::execute (function *fun) > if (something_changed) > { > free_dominance_info (CDI_POST_DOMINATORS); > - /* As we introduced new control-flow we need to insert PHI-nodes > - for the call-clobbers of the remaining call. */ > - mark_virtual_operands_for_renaming (fun); > return TODO_update_ssa; > } > >
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, Oct 30, 2015 at 12:18 PM, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> It's fairly easy to update the virtual ops when the call has no EH edges, >> which should be cheaper than mark_virtual_operands_for_renaming. >> >> Tested on x86_64-linux-gnu, arm-linux-gnueabi and aarch64-linux-gnu. >> OK to install? > > Well. I think this can be easily improved to handle the EH edge case > by not replacing the virtual uses in the EH region. OK. I suppose I was just going for the low-hanging fruit. :-) I'll drop this in favour of getting the internal function stuff finished for stage 1. > Btw, did you verify the pass does things correctly when facing an EH > throwing situation? It seems all math builtins are marked as NOTHROW > regardless of -fnon-call-exceptions ... The main test case for that seems to be g++.dg/opt/pr58165.C. I'd tried other variations of that locally. Thanks, Richard
On Fri, Oct 30, 2015 at 1:21 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Fri, Oct 30, 2015 at 12:18 PM, Richard Sandiford >> <richard.sandiford@arm.com> wrote: >>> It's fairly easy to update the virtual ops when the call has no EH edges, >>> which should be cheaper than mark_virtual_operands_for_renaming. >>> >>> Tested on x86_64-linux-gnu, arm-linux-gnueabi and aarch64-linux-gnu. >>> OK to install? >> >> Well. I think this can be easily improved to handle the EH edge case >> by not replacing the virtual uses in the EH region. > > OK. I suppose I was just going for the low-hanging fruit. :-) Heh, true. It's never that simple. > I'll drop this in favour of getting the internal function stuff finished for stage 1. Fair enough (and yes, I definitely like to see at least internal function support for genmatch/match.pd for GCC 6). >> Btw, did you verify the pass does things correctly when facing an EH >> throwing situation? It seems all math builtins are marked as NOTHROW >> regardless of -fnon-call-exceptions ... > > The main test case for that seems to be g++.dg/opt/pr58165.C. I'd tried > other variations of that locally. Looking at this example I realize it would be nice to be in "(virtual) EH-closed SSA form" so that all uses on an EH edge are reachable by walking the EH edge destination PHI nodes ... Otherwise of couse the fix is to propagate only into uses which are not dominated by the EH dest. A replace_uses_by variant which takes an edge to check ignored uses would come handy here. Richard. > Thanks, > Richard >
diff --git a/gcc/tree-call-cdce.c b/gcc/tree-call-cdce.c index 72828dd..dcaa974 100644 --- a/gcc/tree-call-cdce.c +++ b/gcc/tree-call-cdce.c @@ -708,6 +708,23 @@ gen_shrink_wrap_conditions (gcall *bi_call, vec<gimple *> conds, /* Probability of the branch (to the call) is taken. */ #define ERR_PROB 0.01 +/* Replace BI_CALL's vdef with a phi that uses BI_CALL's definition + when WITH_CALL is taken and the previous definition when WITHOUT_CALL + is taken. JOIN_BB is the target of both edges. */ + +static void +join_vdefs (gcall *bi_call, edge with_call, edge without_call, + basic_block join_bb) +{ + tree old_vuse = gimple_vuse (bi_call); + tree old_vdef = gimple_vdef (bi_call); + tree new_vdef = copy_ssa_name (old_vuse); + gphi *phi = create_phi_node (new_vdef, join_bb); + replace_uses_by (old_vdef, new_vdef); + add_phi_arg (phi, old_vuse, without_call, UNKNOWN_LOCATION); + add_phi_arg (phi, old_vdef, with_call, UNKNOWN_LOCATION); +} + /* The function to shrink wrap a partially dead builtin call whose return value is not used anywhere, but has to be kept live due to potential error condition. Returns true if the @@ -764,7 +781,8 @@ shrink_wrap_one_built_in_call (gcall *bi_call) bi_call_bb = gimple_bb (bi_call); /* Now find the join target bb -- split bi_call_bb if needed. */ - if (stmt_ends_bb_p (bi_call)) + bool call_must_end_bb_p = stmt_ends_bb_p (bi_call); + if (call_must_end_bb_p) { /* If the call must be the last in the bb, don't split the block, it could e.g. have EH edges. */ @@ -822,6 +840,12 @@ shrink_wrap_one_built_in_call (gcall *bi_call) join_tgt_in_edge_fall_thru->count = guard_bb->count - bi_call_in_edge0->count; + if (call_must_end_bb_p) + mark_virtual_operands_for_renaming (cfun); + else + join_vdefs (bi_call, join_tgt_in_edge_from_call, + join_tgt_in_edge_fall_thru, join_tgt_bb); + /* Code generation for the rest of the conditions */ basic_block prev_guard_bb = NULL; while (nconds > 0) @@ -978,9 +1002,6 @@ pass_call_cdce::execute (function *fun) if (something_changed) { free_dominance_info (CDI_POST_DOMINATORS); - /* As we introduced new control-flow we need to insert PHI-nodes - for the call-clobbers of the remaining call. */ - mark_virtual_operands_for_renaming (fun); return TODO_update_ssa; }