Message ID | 5885cfa5-628d-598e-be9e-30292a7de93a@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PR,rtl-optimization/81308] Conditionally cleanup the CFG after insn splitting | expand |
On Mon, Jan 8, 2018 at 5:22 AM, Jeff Law <law@redhat.com> wrote: > > This patch fixes the original problem reported in 81308. Namely that > g++.dg/pr62079.C will trigger a checking failure on 32bit x86. > > As Jakub noted in the BZ the problem is we had an insn with an EH region > note. That insn gets split and the split insns do not have an EH region > note (nor do they need one AFAICT). > > With the EH region note gone, the actual EH region becomes unreachable > and we get a verification error in the dominance code. > > My solution is relatively simple. During splitting we track if the > current insn has an EH region note. If it does and we end splitting the > insn or deleting it as a nop-move, then we note that we're going to need > a cfg cleanup. > > After splitting all insns, we conditionally cleanup the CFG. > > This should keep the overhead relatively low -- primarily it's the cost > to look for the EH region note on each insn as I don't expect the cfg > cleanup is often needed. > > If we could prove to ourselves that the situation only occurs with > -fnon-call-exceptions, then we could further reduce the overhead by > exploiting that invariant as well. I haven't really thought too much > about this. > > No new testcase as the existing pr62079.C will trigger on x86. > > Bootstrapped and regression tested on x86_64. Verified pr62079.C now > passes by hand in 32 bit mode. > > OK for the trunk? Ok. Richard. > Jeff > > PR rtl-optimization/81308 > * recog.c (split_all_insns): Conditionally cleanup the CFG after > splitting insns. > > diff --git a/gcc/recog.c b/gcc/recog.c > index d6aa903..cc28b71 100644 > --- a/gcc/recog.c > +++ b/gcc/recog.c > @@ -2931,6 +2931,7 @@ void > split_all_insns (void) > { > bool changed; > + bool need_cfg_cleanup = false; > basic_block bb; > > auto_sbitmap blocks (last_basic_block_for_fn (cfun)); > @@ -2949,6 +2950,18 @@ split_all_insns (void) > CODE_LABELS and short-out basic blocks. */ > next = NEXT_INSN (insn); > finish = (insn == BB_END (bb)); > + > + /* If INSN has a REG_EH_REGION note and we split INSN, the > + resulting split may not have/need REG_EH_REGION notes. > + > + If that happens and INSN was the last reference to the > + given EH region, then the EH region will become unreachable. > + We can not leave the unreachable blocks in the CFG as that > + will trigger a checking failure. > + > + So track if INSN has a REG_EH_REGION note. If so and we > + split INSN, then trigger a CFG cleanup. */ > + rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX); > if (INSN_P (insn)) > { > rtx set = single_set (insn); > @@ -2965,6 +2978,8 @@ split_all_insns (void) > nops then anyways. */ > if (reload_completed) > delete_insn_and_edges (insn); > + if (note) > + need_cfg_cleanup = true; > } > else > { > @@ -2972,6 +2987,8 @@ split_all_insns (void) > { > bitmap_set_bit (blocks, bb->index); > changed = true; > + if (note) > + need_cfg_cleanup = true; > } > } > } > @@ -2980,7 +2997,16 @@ split_all_insns (void) > > default_rtl_profile (); > if (changed) > - find_many_sub_basic_blocks (blocks); > + { > + find_many_sub_basic_blocks (blocks); > + > + /* Splitting could drop an REG_EH_REGION if it potentially > + trapped in its original form, but does not in its split > + form. Consider a FLOAT_TRUNCATE which splits into a memory > + store/load pair and -fnon-call-exceptions. */ > + if (need_cfg_cleanup) > + cleanup_cfg (0); > + } > > checking_verify_flow_info (); > } >
diff --git a/gcc/recog.c b/gcc/recog.c index d6aa903..cc28b71 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -2931,6 +2931,7 @@ void split_all_insns (void) { bool changed; + bool need_cfg_cleanup = false; basic_block bb; auto_sbitmap blocks (last_basic_block_for_fn (cfun)); @@ -2949,6 +2950,18 @@ split_all_insns (void) CODE_LABELS and short-out basic blocks. */ next = NEXT_INSN (insn); finish = (insn == BB_END (bb)); + + /* If INSN has a REG_EH_REGION note and we split INSN, the + resulting split may not have/need REG_EH_REGION notes. + + If that happens and INSN was the last reference to the + given EH region, then the EH region will become unreachable. + We can not leave the unreachable blocks in the CFG as that + will trigger a checking failure. + + So track if INSN has a REG_EH_REGION note. If so and we + split INSN, then trigger a CFG cleanup. */ + rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX); if (INSN_P (insn)) { rtx set = single_set (insn); @@ -2965,6 +2978,8 @@ split_all_insns (void) nops then anyways. */ if (reload_completed) delete_insn_and_edges (insn); + if (note) + need_cfg_cleanup = true; } else { @@ -2972,6 +2987,8 @@ split_all_insns (void) { bitmap_set_bit (blocks, bb->index); changed = true; + if (note) + need_cfg_cleanup = true; } } } @@ -2980,7 +2997,16 @@ split_all_insns (void) default_rtl_profile (); if (changed) - find_many_sub_basic_blocks (blocks); + { + find_many_sub_basic_blocks (blocks); + + /* Splitting could drop an REG_EH_REGION if it potentially + trapped in its original form, but does not in its split + form. Consider a FLOAT_TRUNCATE which splits into a memory + store/load pair and -fnon-call-exceptions. */ + if (need_cfg_cleanup) + cleanup_cfg (0); + } checking_verify_flow_info (); }