[PATCH][combine] Don't create LSHIFTRT of zero bits in change_zero_ext

[PATCH][combine] Don't create LSHIFTRT of zero bits in change_zero_ext

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Hi all,

At the end of combine if it fails to recognise the patterns it produces it tries
again after transforming all zero_extends and zero_extracts into and-immediate plus
an LSHIFTRT if needed. However, it will construct an LSHIFTRT inside the AND even
if the shift distance is 0, which can hurt recognisability.

This patch fixes that by not creating the LSHIFRT of zero.
I hit this when I was experimenting with some new backend patterns and other unrelated
combine changes, so I don't have a testcase that shows this on a clean trunk, but
I believe this could hurt recognisability in the future.

Bootstrapped and tested on arm, aarch64, x86_64.
I didn't see any codegen differences with clean trunk on aarch64 SPEC2006.

I'm okay with delaying this for next stage 1 if people prefer, though I think it's
pretty low risk.

What do people think?

Kyrill

2015-12-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * combine.c (change_zero_ext): Do not create a shift of zero length.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-shift.patch --]
[-- Type: text/x-patch; name=combine-shift.patch, Size: 579 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index 7d4ffbcc766113c0af1c903f3d0dadbe74dec7fa..2628e1a437c2cd63d439a3ad28d1799b8c679be3 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -11037,7 +11037,9 @@ change_zero_ext (rtx *src)
 	  if (BITS_BIG_ENDIAN)
 	    start = GET_MODE_PRECISION (mode) - size - start;
 
-	  x = gen_rtx_LSHIFTRT (mode, XEXP (x, 0), GEN_INT (start));
+	  x = XEXP (x, 0);
+	  if (start > 0)
+	    x = gen_rtx_LSHIFTRT (mode, x, GEN_INT (start));
 	}
       else if (GET_CODE (x) == ZERO_EXTEND
 	       && GET_CODE (XEXP (x, 0)) == SUBREG

Re: [PATCH][combine] Don't create LSHIFTRT of zero bits in change_zero_ext

[Bernd Schmidt]

On 12/10/2015 03:36 PM, Kyrill Tkachov wrote:
> I'm okay with delaying this for next stage 1 if people prefer, though I
> think it's
> pretty low risk.

I think this is something we should fix now.

> +	  x = XEXP (x, 0);
> +	  if (start > 0)
> +	    x = gen_rtx_LSHIFTRT (mode, x, GEN_INT (start));

I think this should just use simplify_shift_const. gen_rtx_FOO should be 
avoided.


Bernd

Re: [PATCH][combine] Don't create LSHIFTRT of zero bits in change_zero_ext

[Segher Boessenkool]

On Thu, Dec 10, 2015 at 05:05:12PM +0100, Bernd Schmidt wrote:
> On 12/10/2015 03:36 PM, Kyrill Tkachov wrote:
> >I'm okay with delaying this for next stage 1 if people prefer, though I
> >think it's
> >pretty low risk.
> 
> I think this is something we should fix now.

I agree.

> >+	  x = XEXP (x, 0);
> >+	  if (start > 0)
> >+	    x = gen_rtx_LSHIFTRT (mode, x, GEN_INT (start));
> 
> I think this should just use simplify_shift_const. gen_rtx_FOO should be 
> avoided.

A lot of combine does that, it really is stuck in the 80's.  I wouldn't
use simplify_shift_const here, but simply simplify_gen_binary.

The patch is okay with or without that change.


Segher

Re: [PATCH][combine] Don't create LSHIFTRT of zero bits in change_zero_ext

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]


On 11/12/15 01:26, Segher Boessenkool wrote:
> On Thu, Dec 10, 2015 at 05:05:12PM +0100, Bernd Schmidt wrote:
>> On 12/10/2015 03:36 PM, Kyrill Tkachov wrote:
>>> I'm okay with delaying this for next stage 1 if people prefer, though I
>>> think it's
>>> pretty low risk.
>> I think this is something we should fix now.
> I agree.
>
>>> +	  x = XEXP (x, 0);
>>> +	  if (start > 0)
>>> +	    x = gen_rtx_LSHIFTRT (mode, x, GEN_INT (start));
>> I think this should just use simplify_shift_const. gen_rtx_FOO should be
>> avoided.
> A lot of combine does that, it really is stuck in the 80's.  I wouldn't
> use simplify_shift_const here, but simply simplify_gen_binary.

In change_zero_ext it also creates the final AND with gen_rtx_AND.
Perhaps that should also be changed to simplify_gen_binary.
But I haven't seen any cases where it causes trouble yet, so we
could fix it up separately.


> The patch is okay with or without that change.

Thanks for the suggestions.
I'll go with simplify_gen_binary.
Here's what I'll be committing.

Thanks,
Kyrill



2015-12-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * combine.c (change_zero_ext): Do not create a shift of zero length.

>
> Segher
>


[-- Attachment #2: combine-ext-zero.patch --]
[-- Type: text/x-patch, Size: 553 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index 9465e5927145e768f1a5fc43ce7c3621033d8aef..8601d8983ce345e2129dd047b3520d98c0582842 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -11037,7 +11037,8 @@ change_zero_ext (rtx *src)
 	  if (BITS_BIG_ENDIAN)
 	    start = GET_MODE_PRECISION (mode) - size - start;
 
-	  x = gen_rtx_LSHIFTRT (mode, XEXP (x, 0), GEN_INT (start));
+	  x = simplify_gen_binary (LSHIFTRT, mode,
+				   XEXP (x, 0), GEN_INT (start));
 	}
       else if (GET_CODE (x) == ZERO_EXTEND
 	       && GET_CODE (XEXP (x, 0)) == SUBREG