| Message ID | 99c482971a4e6d16ff4bfe3b77bb1b890c73545b.1449691897.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
> The meaning of ZERO_EXTRACT depends on BITS_BIG_ENDIAN, not on > BYTES_BIG_ENDIAN. That's correct. > Testing in progress on powerpc64le-linux; if it passes, is this > okay for trunk? > > > Segher > > > 2015-12-09 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/68814 > * rtlanal.c (set_noop_p): Use BITS_BIG_ENDIAN instead of > BYTES_BIG_ENDIAN. > --- > gcc/rtlanal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c > index 67098e5..f893bca 100644 > --- a/gcc/rtlanal.c > +++ b/gcc/rtlanal.c > @@ -1534,7 +1534,7 @@ set_noop_p (const_rtx set) > > if (GET_CODE (dst) == ZERO_EXTRACT) > return rtx_equal_p (XEXP (dst, 0), src) > - && ! BYTES_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > && !side_effects_p (src); > > if (GET_CODE (dst) == STRICT_LOW_PART) I have a hard time figuring out what a SET verifying the condition would really mean. Could you post it and explain where it comes from?
On Wed, Dec 09, 2015 at 10:13:34PM +0100, Eric Botcazou wrote: > > The meaning of ZERO_EXTRACT depends on BITS_BIG_ENDIAN, not on > > BYTES_BIG_ENDIAN. > > That's correct. > > --- a/gcc/rtlanal.c > > +++ b/gcc/rtlanal.c > > @@ -1534,7 +1534,7 @@ set_noop_p (const_rtx set) > > > > if (GET_CODE (dst) == ZERO_EXTRACT) > > return rtx_equal_p (XEXP (dst, 0), src) > > - && ! BYTES_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > > + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > > && !side_effects_p (src); > > > > if (GET_CODE (dst) == STRICT_LOW_PART) > > I have a hard time figuring out what a SET verifying the condition would > really mean. Could you post it and explain where it comes from? Sure! The condition is meant to say "if you set the low part of a reg to be equal to that reg". And it only handles little-endian numbering (many things with zero_extract do). My case comes from the gcc.dg/pr65492-2.c, the "test1int2" case. combine has made an insn modifying insn i3 21: zero_extract(%3:DI,0x20,0)=%3:DI which is splatting the SImode parameter to both the high and low halves of the dest reg. Then, it tries to combine it with the USE of that reg at the end of the function, giving Failed to match this instruction: (parallel [ (use (ior:DI (and:DI (reg/i:DI 3 3) (const_int 4294967295 [0xffffffff])) (ashift:DI (reg:DI 3 3 [ c ]) (const_int 32 [0x20])))) (set (zero_extract:DI (reg/i:DI 3 3) (const_int 32 [0x20]) (const_int 0 [0])) (reg:DI 3 3 [ c ])) ]) and if it has a parallel of two which doesn't match, it sees if it just needs one arm because the other is a noop set, and that ends up with deleting noop move 21 because of the wrong test, making the testcase fail. (powerpc64le has BITS_BIG_ENDIAN set, a bit unusual). Segher
> My case comes from the gcc.dg/pr65492-2.c, the "test1int2" case. > combine has made an insn > modifying insn i3 21: zero_extract(%3:DI,0x20,0)=%3:DI > which is splatting the SImode parameter to both the high and low halves > of the dest reg. Then, it tries to combine it with the USE of that reg > at the end of the function, giving > > Failed to match this instruction: > (parallel [ > (use (ior:DI (and:DI (reg/i:DI 3 3) > (const_int 4294967295 [0xffffffff])) > (ashift:DI (reg:DI 3 3 [ c ]) > (const_int 32 [0x20])))) > (set (zero_extract:DI (reg/i:DI 3 3) > (const_int 32 [0x20]) > (const_int 0 [0])) > (reg:DI 3 3 [ c ])) > ]) > > and if it has a parallel of two which doesn't match, it sees if it > just needs one arm because the other is a noop set, and that ends > up with > deleting noop move 21 > because of the wrong test, making the testcase fail. > (powerpc64le has BITS_BIG_ENDIAN set, a bit unusual). Thanks. It seems a little odd for the condition to test the POS (operand #2) and not also the LEN (operand #1) of the ZERO_EXTRACT before returning true, but I'm not sure what the test would be given the above example. Or maybe it's implicitly contained in the POS test because of the little-endian case. The patch is OK for mainline.
On Thu, Dec 10, 2015 at 01:26:06PM +0100, Eric Botcazou wrote: > Thanks. It seems a little odd for the condition to test the POS (operand #2) > and not also the LEN (operand #1) of the ZERO_EXTRACT before returning true, > but I'm not sure what the test would be given the above example. Or maybe > it's implicitly contained in the POS test because of the little-endian case. The test looks weird because it only handles the LE case. What it says is simply "if copying the rightmost part of a register to itself", and it then does not matter how many bits are copied. This really does not belong here I'd say (whatever creates this RTL should already simplify it), but I'm just fixing a bug ;-) Segher
Hi, This now also shows up on GCC 5, see PR70672. It there happens in the jump1 pass already. Is this okay to backport to 5 and 4.9? Segher On Wed, Dec 09, 2015 at 08:17:51PM +0000, Segher Boessenkool wrote: > The meaning of ZERO_EXTRACT depends on BITS_BIG_ENDIAN, not on > BYTES_BIG_ENDIAN. This caused PR68814. > > Testing in progress on powerpc64le-linux; if it passes, is this > okay for trunk? > > > Segher > > > 2015-12-09 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/68814 > * rtlanal.c (set_noop_p): Use BITS_BIG_ENDIAN instead of > BYTES_BIG_ENDIAN. > --- > gcc/rtlanal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c > index 67098e5..f893bca 100644 > --- a/gcc/rtlanal.c > +++ b/gcc/rtlanal.c > @@ -1534,7 +1534,7 @@ set_noop_p (const_rtx set) > > if (GET_CODE (dst) == ZERO_EXTRACT) > return rtx_equal_p (XEXP (dst, 0), src) > - && ! BYTES_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > && !side_effects_p (src); > > if (GET_CODE (dst) == STRICT_LOW_PART) > -- > 1.9.3
On 04/15/2016 02:35 AM, Segher Boessenkool wrote: > This now also shows up on GCC 5, see PR70672. It there happens in > the jump1 pass already. > > Is this okay to backport to 5 and 4.9? Ok. Bernd
On Fri, Apr 15, 2016 at 01:37:07PM +0200, Bernd Schmidt wrote: > On 04/15/2016 02:35 AM, Segher Boessenkool wrote: > >This now also shows up on GCC 5, see PR70672. It there happens in > >the jump1 pass already. > > > >Is this okay to backport to 5 and 4.9? > > Ok. Can you please also create a runtime testcase from PR70672 (unless it is roughly the same as the other PR) and commit to all branches? Jakub
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c index 67098e5..f893bca 100644 --- a/gcc/rtlanal.c +++ b/gcc/rtlanal.c @@ -1534,7 +1534,7 @@ set_noop_p (const_rtx set) if (GET_CODE (dst) == ZERO_EXTRACT) return rtx_equal_p (XEXP (dst, 0), src) - && ! BYTES_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx && !side_effects_p (src); if (GET_CODE (dst) == STRICT_LOW_PART)