[PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Segher Boessenkool]

If we have

	(truncate:M1 (and:M2 (lshiftrt:M2 (x:M2) C) C2))

we can write it instead as

	(and:M1 (lshiftrt:M1 (truncate:M1 (x:M2)) C) C2)

(if that is valid, of course), which has smaller modes for the
binary ops, and the truncate can often simplify further (if "x"
is a register, for example).

This fixes gcc.target/powerpc/20050603-3.c for -m32 -mpowerpc64;
also that test is currently restricted to ilp32, but we can run
it with lp64 just fine, in which case it fixes that, too.

Bootstrapped and tested on powerpc64-linux (-m32,-m32/-mpowerpc64,-m64).
Is this okay for trunk?


Segher


2015-11-09  Segher Boessenkool  <segher@kernel.crashing.org>

	* gcc/simplify-rtx.c (simplify_truncation): Simplify TRUNCATE
	of AND of [LA]SHIFTRT.

---
 gcc/simplify-rtx.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 17568ba..1adb393 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -714,6 +714,31 @@ simplify_truncation (machine_mode mode, rtx op,
     return simplify_gen_binary (ASHIFT, mode,
 				XEXP (XEXP (op, 0), 0), XEXP (op, 1));
 
+  /* Likewise (truncate:QI (and:SI (lshiftrt:SI (x:SI) C) C2)) into
+     (and:QI (lshiftrt:QI (truncate:QI (x:SI)) C) C2) for suitable C
+     and C2.  */
+  if (GET_CODE (op) == AND
+      && (GET_CODE (XEXP (op, 0)) == LSHIFTRT
+	  || GET_CODE (XEXP (op, 0)) == ASHIFTRT)
+      && CONST_INT_P (XEXP (XEXP (op, 0), 1))
+      && CONST_INT_P (XEXP (op, 1))
+      && UINTVAL (XEXP (XEXP (op, 0), 1)) < precision
+      && ((GET_MODE_MASK (mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
+	  & UINTVAL (XEXP (op, 1)))
+	 == ((GET_MODE_MASK (op_mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
+	     & UINTVAL (XEXP (op, 1))))
+    {
+      rtx op0 = simplify_gen_unary (TRUNCATE, mode, XEXP (XEXP (op, 0), 0),
+				    op_mode);
+      if (op0)
+	{
+	  op0 = simplify_gen_binary (LSHIFTRT, mode, op0,
+				     XEXP (XEXP (op, 0), 1));
+	  if (op0)
+	    return simplify_gen_binary (AND, mode, op0, XEXP (op, 1));
+	}
+    }
+
   /* Recognize a word extraction from a multi-word subreg.  */
   if ((GET_CODE (op) == LSHIFTRT
        || GET_CODE (op) == ASHIFTRT)
-- 
1.9.3

[PATCH 2/2] rs6000: Extend 20050603-3.c testcase to 64-bit

[Segher Boessenkool]

The testcase used to fail on 64-bit, but it was disabled there.
This patch makes it run there, and beefs up the checking of the
generated code a bit.

Tested on powerpc64-linux *-m32,-m32/-mpowerpc64,-m64).
Is this okay for trunk?


Segher


2015-11-09  Segher Boessenkool  <segher@kernel.crashing.org>

gcc/testsuite/
	* gcc.target/powerpc/20050603-3.c: Don't restrict to ilp32.  Do more
	tests for the expected generated code.

---
 gcc/testsuite/gcc.target/powerpc/20050603-3.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gcc.target/powerpc/20050603-3.c b/gcc/testsuite/gcc.target/powerpc/20050603-3.c
index 0f328e1..4017d34 100644
--- a/gcc/testsuite/gcc.target/powerpc/20050603-3.c
+++ b/gcc/testsuite/gcc.target/powerpc/20050603-3.c
@@ -1,15 +1,19 @@
-/* { dg-do compile { target { ilp32 } } } */
+/* { dg-do compile } */
 /* { dg-options "-O2" } */
-struct Q 
+struct Q
 {
   long x:20;
   long y:4;
   long z:8;
 }b;
-/* This should generate a single rl[w]imi. */
+/* This should generate a single rl[wd]imi. */
 void rotins (unsigned int x)
 {
   b.y = (x<<12) | (x>>20);
 }
 
-/* { dg-final { scan-assembler-not "inm" } } */
+/* { dg-final { scan-assembler-not {\mrlwinm} } } */
+/* { dg-final { scan-assembler-not {\mrldic} } } */
+/* { dg-final { scan-assembler-not {\mrot[lr]} } } */
+/* { dg-final { scan-assembler-not {\ms[lr][wd]} } } */
+/* { dg-final { scan-assembler-times {\mrl[wd]imi} 1 } } */
-- 
1.9.3

Re: [PATCH 2/2] rs6000: Extend 20050603-3.c testcase to 64-bit

[David Edelsohn]

On Sun, Nov 8, 2015 at 11:33 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> The testcase used to fail on 64-bit, but it was disabled there.
> This patch makes it run there, and beefs up the checking of the
> generated code a bit.
>
> Tested on powerpc64-linux *-m32,-m32/-mpowerpc64,-m64).
> Is this okay for trunk?
>
>
> Segher
>
>
> 2015-11-09  Segher Boessenkool  <segher@kernel.crashing.org>
>
> gcc/testsuite/
>         * gcc.target/powerpc/20050603-3.c: Don't restrict to ilp32.  Do more
>         tests for the expected generated code.

This is okay when the simplify-rtx.c patch is approved.

Thanks, David

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Bernd Schmidt]

On 11/09/2015 08:33 AM, Segher Boessenkool wrote:
> If we have
>
> 	(truncate:M1 (and:M2 (lshiftrt:M2 (x:M2) C) C2))
>
> we can write it instead as
>
> 	(and:M1 (lshiftrt:M1 (truncate:M1 (x:M2)) C) C2)
>
>
> +  /* Likewise (truncate:QI (and:SI (lshiftrt:SI (x:SI) C) C2)) into
> +     (and:QI (lshiftrt:QI (truncate:QI (x:SI)) C) C2) for suitable C
> +     and C2.  */
> +  if (GET_CODE (op) == AND
> +      && (GET_CODE (XEXP (op, 0)) == LSHIFTRT
> +	  || GET_CODE (XEXP (op, 0)) == ASHIFTRT)
> +      && CONST_INT_P (XEXP (XEXP (op, 0), 1))
> +      && CONST_INT_P (XEXP (op, 1))
> +      && UINTVAL (XEXP (XEXP (op, 0), 1)) < precision
> +      && ((GET_MODE_MASK (mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
> +	  & UINTVAL (XEXP (op, 1)))
> +	 == ((GET_MODE_MASK (op_mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
> +	     & UINTVAL (XEXP (op, 1))))

In general this would be easier to read if there were intermediate 
variables called shift_amount and mask. I'm not entirely sure what the 
last condition here is supposed to test. Is it related to...

> +	    return simplify_gen_binary (AND, mode, op0, XEXP (op, 1));

... the fact that here I think you'd have to trunc_int_for_mode the AND 
amount for the smaller mode?


Bernd

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Segher Boessenkool]

On Tue, Nov 10, 2015 at 12:16:09PM +0100, Bernd Schmidt wrote:
> On 11/09/2015 08:33 AM, Segher Boessenkool wrote:
> >If we have
> >
> >	(truncate:M1 (and:M2 (lshiftrt:M2 (x:M2) C) C2))
> >
> >we can write it instead as
> >
> >	(and:M1 (lshiftrt:M1 (truncate:M1 (x:M2)) C) C2)
> >
> >
> >+  /* Likewise (truncate:QI (and:SI (lshiftrt:SI (x:SI) C) C2)) into
> >+     (and:QI (lshiftrt:QI (truncate:QI (x:SI)) C) C2) for suitable C
> >+     and C2.  */
> >+  if (GET_CODE (op) == AND
> >+      && (GET_CODE (XEXP (op, 0)) == LSHIFTRT
> >+	  || GET_CODE (XEXP (op, 0)) == ASHIFTRT)
> >+      && CONST_INT_P (XEXP (XEXP (op, 0), 1))
> >+      && CONST_INT_P (XEXP (op, 1))
> >+      && UINTVAL (XEXP (XEXP (op, 0), 1)) < precision
> >+      && ((GET_MODE_MASK (mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
> >+	  & UINTVAL (XEXP (op, 1)))
> >+	 == ((GET_MODE_MASK (op_mode) >> UINTVAL (XEXP (XEXP (op, 0), 1)))
> >+	     & UINTVAL (XEXP (op, 1))))
> 
> In general this would be easier to read if there were intermediate 
> variables called shift_amount and mask.

Yes I know.  All the rest of the code around is it like this though.
Do you want this written in a saner way?

> I'm not entirely sure what the 
> last condition here is supposed to test.

It tests whether moving the truncate inside will give the same result.
It essentially looks if it works for an x with all bits set; if that
works, it works for any x.

> Is it related to...
> 
> >+	    return simplify_gen_binary (AND, mode, op0, XEXP (op, 1));
> 
> ... the fact that here I think you'd have to trunc_int_for_mode the AND 
> amount for the smaller mode?

Ugh yes, I still have to do that for it to be valid RTL in all cases.
Thanks for catching it.


Segher

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Bernd Schmidt]

On 11/10/2015 06:44 PM, Segher Boessenkool wrote:

> Yes I know.  All the rest of the code around is it like this though.
> Do you want this written in a saner way?

I won't object to leaving it as-is for now, but in the future it would 
be good to keep this in mind.

>> I'm not entirely sure what the
>> last condition here is supposed to test.
>
> It tests whether moving the truncate inside will give the same result.
> It essentially looks if it works for an x with all bits set; if that
> works, it works for any x.

Yeah, I figured afterwards that must have been the purpose of the test 
but I was thinking of other constants because of the trunc_int_for_mode 
thing. (I probably would have written "(and_const >> shift_amount) & 
~small_mask == 0" but yours should be ok too). You might want to use 
your description as a comment.

>> ... the fact that here I think you'd have to trunc_int_for_mode the AND
>> amount for the smaller mode?
>
> Ugh yes, I still have to do that for it to be valid RTL in all cases.
> Thanks for catching it.

So FAOD the patch is OK with that change.


Bernd

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Segher Boessenkool]

On Tue, Nov 10, 2015 at 10:04:30PM +0100, Bernd Schmidt wrote:
> On 11/10/2015 06:44 PM, Segher Boessenkool wrote:
> 
> >Yes I know.  All the rest of the code around is it like this though.
> >Do you want this written in a saner way?
> 
> I won't object to leaving it as-is for now, but in the future it would 
> be good to keep this in mind.

With the trunc_int_for_mode it ended up hugging the righthand margin,
so I did clean it up after all.  Please see attached (committed).


Segher


diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 17568ba..c4fc42a 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -714,6 +714,34 @@ simplify_truncation (machine_mode mode, rtx op,
     return simplify_gen_binary (ASHIFT, mode,
 				XEXP (XEXP (op, 0), 0), XEXP (op, 1));
 
+  /* Likewise (truncate:QI (and:SI (lshiftrt:SI (x:SI) C) C2)) into
+     (and:QI (lshiftrt:QI (truncate:QI (x:SI)) C) C2) for suitable C
+     and C2.  */
+  if (GET_CODE (op) == AND
+      && (GET_CODE (XEXP (op, 0)) == LSHIFTRT
+	  || GET_CODE (XEXP (op, 0)) == ASHIFTRT)
+      && CONST_INT_P (XEXP (XEXP (op, 0), 1))
+      && CONST_INT_P (XEXP (op, 1)))
+    {
+      rtx op0 = (XEXP (XEXP (op, 0), 0));
+      rtx shift_op = XEXP (XEXP (op, 0), 1);
+      rtx mask_op = XEXP (op, 1);
+      unsigned HOST_WIDE_INT shift = UINTVAL (shift_op);
+      unsigned HOST_WIDE_INT mask = UINTVAL (mask_op);
+
+      if (shift < precision
+	  /* If doing this transform works for an X with all bits set,
+	     it works for any X.  */
+	  && ((GET_MODE_MASK (mode) >> shift) & mask)
+	     == ((GET_MODE_MASK (op_mode) >> shift) & mask)
+	  && (op0 = simplify_gen_unary (TRUNCATE, mode, op0, op_mode))
+	  && (op0 = simplify_gen_binary (LSHIFTRT, mode, op0, shift_op)))
+	{
+	  mask_op = GEN_INT (trunc_int_for_mode (mask, mode));
+	  return simplify_gen_binary (AND, mode, op0, mask_op);
+	}
+    }
+
   /* Recognize a word extraction from a multi-word subreg.  */
   if ((GET_CODE (op) == LSHIFTRT
        || GET_CODE (op) == ASHIFTRT)
-- 
1.9.3

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Uros Bizjak]

Hello!

> 2015-11-09  Segher Boessenkool  <segher@kernel.crashing.org>
>
> * gcc/simplify-rtx.c (simplify_truncation): Simplify TRUNCATE
> of AND of [LA]SHIFTRT.

Revision r230164 (the above patch) regressed:

FAIL: gcc.target/alpha/pr42269-1.c scan-assembler-not addl

on alpha-linux-gnu.

The difference starts in combine, where before the patch, we were able
to combine insns:

(insn 7 6 8 2 (set (reg:DI 82)
        (lshiftrt:DI (reg:DI 81 [ x ])
            (const_int 16 [0x10]))) pr42269-1.c:8 66 {lshrdi3}
     (expr_list:REG_DEAD (reg:DI 81 [ x ])
        (nil)))
(insn 8 7 11 2 (set (reg:DI 70 [ _2 ])
        (sign_extend:DI (subreg:SI (reg:DI 82) 0))) pr42269-1.c:8 2
{*extendsidi2_1}
     (expr_list:REG_DEAD (reg:DI 82)
        (nil)))

to:

Trying 7 -> 8:
Successfully matched this instruction:
(set (reg:DI 70 [ _2 ])
    (zero_extract:DI (reg/v:DI 80 [ x ])
        (const_int 16 [0x10])
        (const_int 16 [0x10])))
allowing combination of insns 7 and 8
original costs 4 + 4 = 8
replacement cost 4
deferring deletion of insn with uid = 7.
modifying insn i3     8: r70:DI=zero_extract(r80:DI,0x10,0x10)
deferring rescan insn with uid = 8.

After the patch, the combination fails:

Trying 7 -> 8:
Failed to match this instruction:
(set (reg:DI 70 [ _2 ])
    (sign_extend:DI (lshiftrt:SI (subreg:SI (reg/v:DI 80 [ x ]) 0)
            (const_int 16 [0x10]))))

Uros.

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Segher Boessenkool]

Hi!

On Fri, Nov 13, 2015 at 11:02:55AM +0100, Uros Bizjak wrote:
> on alpha-linux-gnu.
> 
> The difference starts in combine, where before the patch, we were able
> to combine insns:
> 
> (insn 7 6 8 2 (set (reg:DI 82)
>         (lshiftrt:DI (reg:DI 81 [ x ])
>             (const_int 16 [0x10]))) pr42269-1.c:8 66 {lshrdi3}
>      (expr_list:REG_DEAD (reg:DI 81 [ x ])
>         (nil)))
> (insn 8 7 11 2 (set (reg:DI 70 [ _2 ])
>         (sign_extend:DI (subreg:SI (reg:DI 82) 0))) pr42269-1.c:8 2
> {*extendsidi2_1}
>      (expr_list:REG_DEAD (reg:DI 82)
>         (nil)))
> 
> to:
> 
> Trying 7 -> 8:
> Successfully matched this instruction:
> (set (reg:DI 70 [ _2 ])
>     (zero_extract:DI (reg/v:DI 80 [ x ])
>         (const_int 16 [0x10])
>         (const_int 16 [0x10])))
> allowing combination of insns 7 and 8
> original costs 4 + 4 = 8
> replacement cost 4
> deferring deletion of insn with uid = 7.
> modifying insn i3     8: r70:DI=zero_extract(r80:DI,0x10,0x10)
> deferring rescan insn with uid = 8.
> 
> After the patch, the combination fails:
> 
> Trying 7 -> 8:
> Failed to match this instruction:
> (set (reg:DI 70 [ _2 ])
>     (sign_extend:DI (lshiftrt:SI (subreg:SI (reg/v:DI 80 [ x ]) 0)
>             (const_int 16 [0x10]))))

Somehow, before the patch, it decided to do a zero-extension (where the
combined insns had a sign extension).  Was that even correct?  Maybe
many bits of reg 80 (or, hrm, 81 in the orig?!) are known zero?


Segher

Re: [PATCH 1/2] simplify-rtx: Simplify trunc of and of shiftrt

[Uros Bizjak]

On Fri, Nov 13, 2015 at 3:36 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> Hi!
>
> On Fri, Nov 13, 2015 at 11:02:55AM +0100, Uros Bizjak wrote:
>> on alpha-linux-gnu.
>>
>> The difference starts in combine, where before the patch, we were able
>> to combine insns:
>>
>> (insn 7 6 8 2 (set (reg:DI 82)
>>         (lshiftrt:DI (reg:DI 81 [ x ])
>>             (const_int 16 [0x10]))) pr42269-1.c:8 66 {lshrdi3}
>>      (expr_list:REG_DEAD (reg:DI 81 [ x ])
>>         (nil)))
>> (insn 8 7 11 2 (set (reg:DI 70 [ _2 ])
>>         (sign_extend:DI (subreg:SI (reg:DI 82) 0))) pr42269-1.c:8 2
>> {*extendsidi2_1}
>>      (expr_list:REG_DEAD (reg:DI 82)
>>         (nil)))
>>
>> to:
>>
>> Trying 7 -> 8:
>> Successfully matched this instruction:
>> (set (reg:DI 70 [ _2 ])
>>     (zero_extract:DI (reg/v:DI 80 [ x ])
>>         (const_int 16 [0x10])
>>         (const_int 16 [0x10])))
>> allowing combination of insns 7 and 8
>> original costs 4 + 4 = 8
>> replacement cost 4
>> deferring deletion of insn with uid = 7.
>> modifying insn i3     8: r70:DI=zero_extract(r80:DI,0x10,0x10)
>> deferring rescan insn with uid = 8.
>>
>> After the patch, the combination fails:
>>
>> Trying 7 -> 8:
>> Failed to match this instruction:
>> (set (reg:DI 70 [ _2 ])
>>     (sign_extend:DI (lshiftrt:SI (subreg:SI (reg/v:DI 80 [ x ]) 0)
>>             (const_int 16 [0x10]))))
>
> Somehow, before the patch, it decided to do a zero-extension (where the
> combined insns had a sign extension).  Was that even correct?  Maybe
> many bits of reg 80 (or, hrm, 81 in the orig?!) are known zero?

Oops, this analysis is wrong. I'll re-do the analysis in reported PR 68330 [1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68330

Uros.