| Message ID | d0d01f953b89393e24ce8d483ba8829705fe7fff.1480509684.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 30, 2016 at 1:46 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > In the testcase, IRA propagates a constant into a TRAP_IF insn, which > then becomes an unconditional trap. Unconditional traps are control > flow insns so doing this requires surgery on the cfg. Huh, that's an odd choice ;) I'd say TRAP_IF should be a control-flow insn as well, but well... > We cannot do > that here, so instead refuse to do the substitution. > > Bootstrapping + regression testing on powerpc64-linux {-m64,-m32} > (the bug happened here with -m32); okay for trunk if this succeeds? > > > Segher > > > 2016-11-30 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/78610 > * ira.c (combine_and_move_insns): Don't substitute into TRAP_IF > instructions. > > gcc/testsuite/ > PR rtl-optimization/78610 > * gcc.c-torture/compile/pr78610.c: New testcase. > > --- > gcc/ira.c | 5 +++++ > gcc/testsuite/gcc.c-torture/compile/pr78610.c | 14 ++++++++++++++ > 2 files changed, 19 insertions(+) > create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr78610.c > > diff --git a/gcc/ira.c b/gcc/ira.c > index d20ec99..ccd4980 100644 > --- a/gcc/ira.c > +++ b/gcc/ira.c > @@ -3669,6 +3669,11 @@ combine_and_move_insns (void) > if (JUMP_P (use_insn)) > continue; > > + /* Also don't substitute into a conditional trap insn -- it can become > + an unconditional trap, and that is a flow control insn. */ > + if (GET_CODE (PATTERN (use_insn)) == TRAP_IF) > + continue; > + > df_ref def = DF_REG_DEF_CHAIN (regno); > gcc_assert (DF_REG_DEF_COUNT (regno) == 1 && DF_REF_INSN_INFO (def)); > rtx_insn *def_insn = DF_REF_INSN (def); > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr78610.c b/gcc/testsuite/gcc.c-torture/compile/pr78610.c > new file mode 100644 > index 0000000..0415ae6 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/compile/pr78610.c > @@ -0,0 +1,14 @@ > +/* PR rtl-optimization/78610 */ > + > +unsigned int ao, gl; > + > +void > +ri (void) > +{ > + for (;;) > + { > + if (ao != 1) > + ao /= 0; > + gl = 0; > + } > +} > -- > 1.9.3 >
On 11/30/2016 05:46 AM, Segher Boessenkool wrote: > In the testcase, IRA propagates a constant into a TRAP_IF insn, which > then becomes an unconditional trap. Unconditional traps are control > flow insns so doing this requires surgery on the cfg. We cannot do > that here, so instead refuse to do the substitution. > > Bootstrapping + regression testing on powerpc64-linux {-m64,-m32} > (the bug happened here with -m32); okay for trunk if this succeeds? > > > Segher > > > 2016-11-30 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/78610 > * ira.c (combine_and_move_insns): Don't substitute into TRAP_IF > instructions. > > gcc/testsuite/ > PR rtl-optimization/78610 > * gcc.c-torture/compile/pr78610.c: New testcase. Funny how you speculated there could be these issues hiding in the weeds, then just a few days later, one crawls out. OK. I do wonder if we're going to need a better mechanism for this in the long term, but OK for now. jeff
On 11/30/2016 05:52 AM, Richard Biener wrote: > On Wed, Nov 30, 2016 at 1:46 PM, Segher Boessenkool > <segher@kernel.crashing.org> wrote: >> In the testcase, IRA propagates a constant into a TRAP_IF insn, which >> then becomes an unconditional trap. Unconditional traps are control >> flow insns so doing this requires surgery on the cfg. > > Huh, that's an odd choice ;) I'd say TRAP_IF should be a control-flow insn > as well, but well... Perhaps. The situation with TRAP_IF reminds me a lot of an indirect call, which after some kind of propagation turns into a direct call to a non-returning function. It transforms from something that is not control flow in the traditional sense to something we do consider control flow. Jeff
On Wed, Nov 30, 2016 at 1:57 PM, Jeff Law <law@redhat.com> wrote: > On 11/30/2016 05:52 AM, Richard Biener wrote: >> >> On Wed, Nov 30, 2016 at 1:46 PM, Segher Boessenkool >> <segher@kernel.crashing.org> wrote: >>> >>> In the testcase, IRA propagates a constant into a TRAP_IF insn, which >>> then becomes an unconditional trap. Unconditional traps are control >>> flow insns so doing this requires surgery on the cfg. >> >> >> Huh, that's an odd choice ;) I'd say TRAP_IF should be a control-flow >> insn >> as well, but well... > > Perhaps. > > The situation with TRAP_IF reminds me a lot of an indirect call, which after > some kind of propagation turns into a direct call to a non-returning > function. It transforms from something that is not control flow in the > traditional sense to something we do consider control flow. OTOH the noreturn flag is now on the GIMPLE stmt via gimple_call_[set_]_ctrl_altering_p and thus the mere transform into a direct call does _not_ make the call noreturn! It only opens up the possibility to optimize it so ;) (fixup-cfg does that) There's also the reverse - an indirect noreturn call transformed to a direct not noreturn call. This is why we have the flag explicit on the IL as well (we keep that call as "noreturn" as we have no idea where to redirect a new fallthru to). Richard. > Jeff >
On Wed, Nov 30, 2016 at 01:52:51PM +0100, Richard Biener wrote: > On Wed, Nov 30, 2016 at 1:46 PM, Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > In the testcase, IRA propagates a constant into a TRAP_IF insn, which > > then becomes an unconditional trap. Unconditional traps are control > > flow insns so doing this requires surgery on the cfg. > > Huh, that's an odd choice ;) I'd say TRAP_IF should be a control-flow insn > as well, but well... It doesn't really matter here, converting a conditional TRAP_IF to an unconditional one requires changing the cfg in any case (and we cannot do that here). Making every TRAP_IF a control flow insn means they will end their BB; that will make some things simpler, sure. It will also limit some of the RTL optimizations a bit. Segher
On Wed, Nov 30, 2016 at 05:54:58AM -0700, Jeff Law wrote: > Funny how you speculated there could be these issues hiding in the > weeds, then just a few days later, one crawls out. Two (there is PR78607 as well). Although that one seems related to the combine one. All the same reporter, it's not a big coincidence ;-) Segher
On 30/11/2016 13:46, Segher Boessenkool wrote: > if (JUMP_P (use_insn)) > continue; > > + /* Also don't substitute into a conditional trap insn -- it can become > + an unconditional trap, and that is a flow control insn. */ > + if (GET_CODE (PATTERN (use_insn)) == TRAP_IF) > + continue; Should there be a predicate that catches JUMP_Ps but also TRAP_IF? Paolo
On Thu, Dec 01, 2016 at 12:24:37PM +0100, Paolo Bonzini wrote: > > > On 30/11/2016 13:46, Segher Boessenkool wrote: > > if (JUMP_P (use_insn)) > > continue; > > > > + /* Also don't substitute into a conditional trap insn -- it can become > > + an unconditional trap, and that is a flow control insn. */ > > + if (GET_CODE (PATTERN (use_insn)) == TRAP_IF) > > + continue; > > Should there be a predicate that catches JUMP_Ps but also TRAP_IF? Maybe. A conditional TRAP_IF is quite unlike a JUMP, and having two separate statements here is handy because I need to put that comment somewhere ;-) Segher
diff --git a/gcc/ira.c b/gcc/ira.c index d20ec99..ccd4980 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -3669,6 +3669,11 @@ combine_and_move_insns (void) if (JUMP_P (use_insn)) continue; + /* Also don't substitute into a conditional trap insn -- it can become + an unconditional trap, and that is a flow control insn. */ + if (GET_CODE (PATTERN (use_insn)) == TRAP_IF) + continue; + df_ref def = DF_REG_DEF_CHAIN (regno); gcc_assert (DF_REG_DEF_COUNT (regno) == 1 && DF_REF_INSN_INFO (def)); rtx_insn *def_insn = DF_REF_INSN (def); diff --git a/gcc/testsuite/gcc.c-torture/compile/pr78610.c b/gcc/testsuite/gcc.c-torture/compile/pr78610.c new file mode 100644 index 0000000..0415ae6 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr78610.c @@ -0,0 +1,14 @@ +/* PR rtl-optimization/78610 */ + +unsigned int ao, gl; + +void +ri (void) +{ + for (;;) + { + if (ao != 1) + ao /= 0; + gl = 0; + } +}
In the testcase, IRA propagates a constant into a TRAP_IF insn, which then becomes an unconditional trap. Unconditional traps are control flow insns so doing this requires surgery on the cfg. We cannot do that here, so instead refuse to do the substitution. Bootstrapping + regression testing on powerpc64-linux {-m64,-m32} (the bug happened here with -m32); okay for trunk if this succeeds? Segher 2016-11-30 Segher Boessenkool <segher@kernel.crashing.org> PR rtl-optimization/78610 * ira.c (combine_and_move_insns): Don't substitute into TRAP_IF instructions. gcc/testsuite/ PR rtl-optimization/78610 * gcc.c-torture/compile/pr78610.c: New testcase. --- gcc/ira.c | 5 +++++ gcc/testsuite/gcc.c-torture/compile/pr78610.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr78610.c