[PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Segher Boessenkool]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Eric Botcazou]

> 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?

-- 
Eric Botcazou

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Segher Boessenkool]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Eric Botcazou]

> 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.

-- 
Eric Botcazou

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Segher Boessenkool]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Segher Boessenkool]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Bernd Schmidt]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Jakub Jelinek]

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

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Segher Boessenkool]

On Fri, Apr 15, 2016 at 01:41:25PM +0200, Jakub Jelinek wrote:
> 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?

I came up with this.  I'll commit it in an hour or so unless someone
complains; it should work on all targets, BE and LE (tested on
powerpc64-linux and powerpc64le-linux).

So I now need to commit it to one more branch, yay :-)


Segher


gcc/testsuite/
	PR rtl-optimization/70672
	PR rtl-optimization/68814
	* gcc.dg/pr70672.c: New testcase.


diff --git a/gcc/testsuite/gcc.dg/pr70672.c b/gcc/testsuite/gcc.dg/pr70672.c
new file mode 100644
index 0000000..d785124
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr70672.c
@@ -0,0 +1,64 @@
+/* PR rtl-optimization/70672 */
+/* PR rtl-optimization/68814 */
+/* { dg-do run { target { stdint_types } } } */
+/* { dg-options "-O2" } */
+
+#include <stdint.h>
+#include <stdlib.h>
+
+struct pair {
+  uint32_t one;
+  uint32_t two;
+};
+
+union u {
+  struct pair pair;
+  uint64_t num;
+};
+
+uint64_t __attribute__ ((noinline))
+f(uint64_t a)
+{
+  union u x;
+
+  x.num = a;
+
+  x.pair.one = x.pair.two;
+  x.pair.two = 0;
+
+  return x.num;
+}
+
+uint64_t __attribute__ ((noinline))
+g(uint64_t a)
+{
+  union u x;
+
+  x.num = a;
+
+  x.pair.two = x.pair.one;
+  x.pair.one = 0;
+
+  return x.num;
+}
+
+int
+main (void)
+{
+  uint64_t x = 0x123456789abcdef0ULL;
+  uint64_t fx = f(x);
+  uint64_t gx = g(x);
+  uint64_t fgx = f(gx);
+  uint64_t gfx = g(fx);
+
+  if ((fx & gx))
+    abort ();
+
+  if ((fgx & gfx))
+    abort ();
+
+  if ((fgx | gfx) != x)
+    abort ();
+
+  return 0;
+}