[PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi,

  This is a simple fix for PR94026.  
  With this fix, combine will try make an extraction if we are in a equality comparison and this is an AND
  with a constant which is power of two minus one.  Shift here should be an constant.  For example, combine
  will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract (x 2 9)) 0).  

  Added one test case for this.  Bootstrap and tested on both x86_64 and aarch64 Linux platform.  
  Any suggestion?  

Thanks,
Felix

gcc:
+2020-03-04  Felix Yang  <felix.yang@huawei.com>
+
+       PR rtl-optimization/94026
+       * combine.c (make_compound_operation_int): Make an extraction
+         if we are in a equality comparison and this is an AND with a
+         constant which is power of two minus one.
+

gcc/testsuite:
+2020-03-04  Felix Yang  <felix.yang@huawei.com>
+
+       PR rtl-optimization/94026
+       * gcc.dg/pr94026.c: New test.
+

[-- Attachment #2: pr94026-v0.txt --]
[-- Type: text/plain, Size: 5277 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index 58366a6d331..c05064fc333 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8170,14 +8170,31 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
-	 is a logical right shift, make an extraction.  */
+	 is a logical right shift, make an extraction.
+	 If we are in a equality comparison and this is an AND with a constant
+	 which is power of two minus one, also make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
-	  && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0)
+	  && (pos == 0 || (pos > 0 && equality_comparison
+			   && CONST_INT_P (XEXP (XEXP (x, 0), 1)))))
 	{
 	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
-	  new_rtx = make_extraction (mode, new_rtx, 0, XEXP (XEXP (x, 0), 1),
-				     i, 1, 0, in_code == COMPARE);
+	  if (pos == 0)
+	    {
+	      new_rtx = make_extraction (mode, new_rtx, 0,
+					 XEXP (XEXP (x, 0), 1), len, 1, 0,
+					 in_code == COMPARE);
+	    }
+	  else
+	    {
+	      int real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	      new_rtx = make_extraction (mode, new_rtx, real_pos, NULL_RTX,
+					 len, 1, 0, in_code == COMPARE);
+	    }
 	}
 
       /* Same as previous, but for (subreg (lshiftrt ...)) in first op.  */
@@ -8186,13 +8203,25 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	       && is_a <scalar_int_mode> (GET_MODE (SUBREG_REG (XEXP (x, 0))),
 					  &inner_mode)
 	       && GET_CODE (SUBREG_REG (XEXP (x, 0))) == LSHIFTRT
-	       && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0)
+	       && (pos == 0
+		   || (pos > 0 && equality_comparison
+		       && CONST_INT_P (XEXP (SUBREG_REG (XEXP (x, 0)), 1)))))
 	{
 	  rtx inner_x0 = SUBREG_REG (XEXP (x, 0));
 	  new_rtx = make_compound_operation (XEXP (inner_x0, 0), next_code);
-	  new_rtx = make_extraction (inner_mode, new_rtx, 0,
-				     XEXP (inner_x0, 1),
-				     i, 1, 0, in_code == COMPARE);
+	  if (pos == 0)
+	    {
+	      new_rtx = make_extraction (inner_mode, new_rtx, 0,
+					 XEXP (inner_x0, 1),
+					 len, 1, 0, in_code == COMPARE);
+	    }
+	  else
+	    {
+	      int real_pos = pos + UINTVAL (XEXP (inner_x0, 1));
+	      new_rtx = make_extraction (inner_mode, new_rtx, real_pos,
+					 NULL_RTX, len, 1, 0,
+					 in_code == COMPARE);
+	    }
 
 	  /* If we narrowed the mode when dropping the subreg, then we lose.  */
 	  if (GET_MODE_SIZE (inner_mode) < GET_MODE_SIZE (mode))
@@ -8200,10 +8229,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 
 	  /* If that didn't give anything, see if the AND simplifies on
 	     its own.  */
-	  if (!new_rtx && i >= 0)
+	  if (!new_rtx)
 	    {
 	      new_rtx = make_compound_operation (XEXP (x, 0), next_code);
-	      new_rtx = make_extraction (mode, new_rtx, 0, NULL_RTX, i, 1,
+	      new_rtx = make_extraction (mode, new_rtx, pos, NULL_RTX, len, 1,
 					 0, in_code == COMPARE);
 	    }
 	}
@@ -8212,7 +8241,7 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 		|| GET_CODE (XEXP (x, 0)) == IOR)
 	       && GET_CODE (XEXP (XEXP (x, 0), 0)) == LSHIFTRT
 	       && GET_CODE (XEXP (XEXP (x, 0), 1)) == LSHIFTRT
-	       && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0)
+	       && (pos == 0 || (pos > 0 && equality_comparison)))
 	{
 	  /* Apply the distributive law, and then try to make extractions.  */
 	  new_rtx = gen_rtx_fmt_ee (GET_CODE (XEXP (x, 0)), mode,
@@ -8228,14 +8257,14 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 
       else if (GET_CODE (XEXP (x, 0)) == ROTATE
 	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
-	       && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0
-	       && i <= INTVAL (XEXP (XEXP (x, 0), 1)))
+	       && (pos == 0 || (pos > 0 && equality_comparison))
+	       && pos + len <= UINTVAL (XEXP (XEXP (x, 0), 1)))
 	{
 	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
 	  new_rtx = make_extraction (mode, new_rtx,
 				     (GET_MODE_PRECISION (mode)
-				      - INTVAL (XEXP (XEXP (x, 0), 1))),
-				     NULL_RTX, i, 1, 0, in_code == COMPARE);
+				      - UINTVAL (XEXP (XEXP (x, 0), 1)) + pos),
+				     NULL_RTX, len, 1, 0, in_code == COMPARE);
 	}
 
       /* On machines without logical shifts, if the operand of the AND is
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 00000000000..7b5184dfb2c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fdump-rtl-combine" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (zero_extract (x 2 9)) 0). We look for the *attempt*
+   to match this RTL pattern, regardless of whether an actual insn
+   may be found on the platform.  */
+/* { dg-final { scan-rtl-dump "\\(zero_extract" "combine" } } */

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Jeff Law]

On Wed, 2020-03-04 at 08:39 +0000, Yangfei (Felix) wrote:
> Hi,
> 
>   This is a simple fix for PR94026.  
>   With this fix, combine will try make an extraction if we are in a equality
> comparison and this is an AND
>   with a constant which is power of two minus one.  Shift here should be an
> constant.  For example, combine
>   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract
> (x 2 9)) 0).  
> 
>   Added one test case for this.  Bootstrap and tested on both x86_64 and
> aarch64 Linux platform.  
>   Any suggestion?  
> 
> Thanks,
> Felix
> 
> gcc:
> +2020-03-04  Felix Yang  <felix.yang@huawei.com>
> +
> +       PR rtl-optimization/94026
> +       * combine.c (make_compound_operation_int): Make an extraction
> +         if we are in a equality comparison and this is an AND with a
> +         constant which is power of two minus one.
> +
> 
> gcc/testsuite:
> +2020-03-04  Felix Yang  <felix.yang@huawei.com>
> +
> +       PR rtl-optimization/94026
> +       * gcc.dg/pr94026.c: New test.
Just a note.  We're in stage4 of our development cycle, meaning we focus on
regression bugfixes.  I've queued this for evaluation in gcc-11.
jeff

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Thursday, March 5, 2020 11:37 PM
> To: Yangfei (Felix) <felix.yang@huawei.com>; gcc-patches@gcc.gnu.org
> Cc: Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> On Wed, 2020-03-04 at 08:39 +0000, Yangfei (Felix) wrote:
> > Hi,
> >
> >   This is a simple fix for PR94026.
> >   With this fix, combine will try make an extraction if we are in a
> > equality comparison and this is an AND
> >   with a constant which is power of two minus one.  Shift here should
> > be an constant.  For example, combine
> >   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare
> > (zero_extract (x 2 9)) 0).
> >
> >   Added one test case for this.  Bootstrap and tested on both x86_64
> > and
> > aarch64 Linux platform.
> >   Any suggestion?
> >
> > Thanks,
> > Felix
> >
> > gcc:
> > +2020-03-04  Felix Yang  <felix.yang@huawei.com>
> > +
> > +       PR rtl-optimization/94026
> > +       * combine.c (make_compound_operation_int): Make an extraction
> > +         if we are in a equality comparison and this is an AND with a
> > +         constant which is power of two minus one.
> > +
> >
> > gcc/testsuite:
> > +2020-03-04  Felix Yang  <felix.yang@huawei.com>
> > +
> > +       PR rtl-optimization/94026
> > +       * gcc.dg/pr94026.c: New test.
> Just a note.  We're in stage4 of our development cycle, meaning we focus on
> regression bugfixes.  I've queued this for evaluation in gcc-11.
> jeff

Sure, this is intended for 11.  Thanks for doing that : - ) 

Best regards,
Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

Hi!

Please Cc: me on combine patches; you sent it nine days ago, and I didn't
see it until now.

On Wed, Mar 04, 2020 at 08:39:36AM +0000, Yangfei (Felix) wrote:
>   This is a simple fix for PR94026.  
>   With this fix, combine will try make an extraction if we are in a equality comparison and this is an AND
>   with a constant which is power of two minus one.  Shift here should be an constant.  For example, combine
>   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract (x 2 9)) 0).  

Why is that a good thing?

(There should be thorough tests on many archs, showing it helps on
average, and it doesn't regress anything.  I can do that for you, but
not right now).

The code needs more comments, and the commit message should say what is
done and why you made those choices.

In general, we should have *fewer* zero_extract, not more.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Friday, March 13, 2020 7:50 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> Hi!
> 
> Please Cc: me on combine patches; you sent it nine days ago, and I didn't see it
> until now.

OK.  

> On Wed, Mar 04, 2020 at 08:39:36AM +0000, Yangfei (Felix) wrote:
> >   This is a simple fix for PR94026.
> >   With this fix, combine will try make an extraction if we are in a equality
> comparison and this is an AND
> >   with a constant which is power of two minus one.  Shift here should be an
> constant.  For example, combine
> >   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract
> (x 2 9)) 0).
> 
> Why is that a good thing?

The reported test case is reduced from spec2017 541.leela_r.  I have pasted original code snippet on the bugzilla.  
We found other compilers like aocc/llvm can catch this pattern and simplify it.  

> (There should be thorough tests on many archs, showing it helps on average,
> and it doesn't regress anything.  I can do that for you, but not right now).

I only have aarch64 & x86_64 linux available and have tested this patch with spec17 on both platforms.  
No obvious improvement & regression witnessed.  This is expected as only one instruction is reduced here.  
It's appreciated if this can be tested on other archs.  

> The code needs more comments, and the commit message should say what is
> done and why you made those choices.
> In general, we should have *fewer* zero_extract, not more.

OK.  I can add more comments & commit message if finally we are inclined to go with this patch.  

Thanks,
Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Fri, Mar 13, 2020 at 03:21:18AM +0000, Yangfei (Felix) wrote:
> > On Wed, Mar 04, 2020 at 08:39:36AM +0000, Yangfei (Felix) wrote:
> > >   This is a simple fix for PR94026.
> > >   With this fix, combine will try make an extraction if we are in a equality
> > comparison and this is an AND
> > >   with a constant which is power of two minus one.  Shift here should be an
> > constant.  For example, combine
> > >   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract
> > (x 2 9)) 0).
> > 
> > Why is that a good thing?
> 
> The reported test case is reduced from spec2017 541.leela_r.  I have pasted original code snippet on the bugzilla.  
> We found other compilers like aocc/llvm can catch this pattern and simplify it.  

That wasn't my question, let me rephrase: why would writing it as
zero_extract (instead of as a more canonical form) be wanted?

The aarch backend only has zero_extract formulations for most of the
bitfield instructions.  If you fix that problem, all of this should go
away?  Like, the testcase in the PR starts with

Trying 7 -> 8:
    7: r99:SI=r103:SI>>r104:SI#0
      REG_DEAD r104:SI
      REG_DEAD r103:SI
    8: r100:SI=r99:SI&0x6
      REG_DEAD r99:SI
Failed to match this instruction:
(set (reg:SI 100)
    (and:SI (ashiftrt:SI (reg:SI 103)
            (subreg:QI (reg:SI 104) 0))
        (const_int 6 [0x6])))

and that should match already (that's an ubfm (ubfx))?


> > (There should be thorough tests on many archs, showing it helps on average,
> > and it doesn't regress anything.  I can do that for you, but not right now).
> 
> I only have aarch64 & x86_64 linux available and have tested this patch with spec17 on both platforms.  
> No obvious improvement & regression witnessed.  This is expected as only one instruction is reduced here.  

What should be tested is what new combinations are done, and which are
*no longer* done.

> > In general, we should have *fewer* zero_extract, not more.

Some reasons for that:

1) All those can be expressed with simpler operations as well;
2) Most very similar expressions cannot be expressed as zero_extract,
although many architectures can handle (some of) those just fine;
3) The optimizers do not handle zero_extract very well at all (this
includes simplify-rtx, to start with).

sign_extract is nastier -- we really want to have a sign_extend that
works on separate bits, not as coarse as address units as we have now --
but it currently isn't handled much either.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Saturday, March 14, 2020 12:07 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> On Fri, Mar 13, 2020 at 03:21:18AM +0000, Yangfei (Felix) wrote:
> > > On Wed, Mar 04, 2020 at 08:39:36AM +0000, Yangfei (Felix) wrote:
> > > >   This is a simple fix for PR94026.
> > > >   With this fix, combine will try make an extraction if we are in
> > > > a equality
> > > comparison and this is an AND
> > > >   with a constant which is power of two minus one.  Shift here
> > > > should be an
> > > constant.  For example, combine
> > > >   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare
> > > > (zero_extract
> > > (x 2 9)) 0).
> > >
> > > Why is that a good thing?
> >
> > The reported test case is reduced from spec2017 541.leela_r.  I have pasted
> original code snippet on the bugzilla.
> > We found other compilers like aocc/llvm can catch this pattern and simplify it.
> 
> That wasn't my question, let me rephrase: why would writing it as zero_extract
> (instead of as a more canonical form) be wanted?

Sorry for not getting your point here. 

> The aarch backend only has zero_extract formulations for most of the bitfield
> instructions.  If you fix that problem, all of this should go away?  Like, the
> testcase in the PR starts with
> 
> Trying 7 -> 8:
>     7: r99:SI=r103:SI>>r104:SI#0
>       REG_DEAD r104:SI
>       REG_DEAD r103:SI
>     8: r100:SI=r99:SI&0x6
>       REG_DEAD r99:SI
> Failed to match this instruction:
> (set (reg:SI 100)
>     (and:SI (ashiftrt:SI (reg:SI 103)
>             (subreg:QI (reg:SI 104) 0))
>         (const_int 6 [0x6])))
> 
> and that should match already (that's an ubfm (ubfx))?

For aarch64, if we use "ubfm/ubfx" for the reduced test case, then we still need to do a compare with zero.  Then we won't get the benefit.  
For aarch64, we need to emit a "tst" instruction here.  So we need to catch something like:  

149 (set (reg:CC_NZ 66 cc)
150     (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 102)
151                 (const_int 8 [0x8]))
152             (const_int 6 [0x6]))
153         (const_int 0 [0])))

But this pattern is not accurate enough: we can only accept equality comparison with zero here (as indicated by the checking of equality_comparison in my original patch).  
Also, this issue is there for ports like x86.  If we go that way, then we need to handle each port affected.  
So I am inclined to handle this in an arch-independent way.  
I looked into tree phases like fwprop & fold-const before, but didn't see an appropriate point to catch this opportunity.  
Then I came to the combine phase.  

> 
> > > (There should be thorough tests on many archs, showing it helps on
> > > average, and it doesn't regress anything.  I can do that for you, but not
> right now).
> >
> > I only have aarch64 & x86_64 linux available and have tested this patch with
> spec17 on both platforms.
> > No obvious improvement & regression witnessed.  This is expected as only
> one instruction is reduced here.
> 
> What should be tested is what new combinations are done, and which are *no
> longer* done.

In theory, we won't lose but emit more zero_extract with my patch.  

> > > In general, we should have *fewer* zero_extract, not more.
> 
> Some reasons for that:
> 
> 1) All those can be expressed with simpler operations as well;
> 2) Most very similar expressions cannot be expressed as zero_extract,
> although many architectures can handle (some of) those just fine;
> 3) The optimizers do not handle zero_extract very well at all (this includes
> simplify-rtx, to start with).
> 
> sign_extract is nastier -- we really want to have a sign_extend that works on
> separate bits, not as coarse as address units as we have now -- but it currently
> isn't handled much either.

Thanks for explaining this.  I have to admit that I didn't realize this issue when I was creating my original patch.  


Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Mon, Mar 16, 2020 at 06:29:39AM +0000, Yangfei (Felix) wrote:
> Sorry for not getting your point here. 

Trying 7 -> 8:
    7: r99:SI=r103:SI>>0x8
      REG_DEAD r103:SI
    8: r100:SI=r99:SI&0x6
      REG_DEAD r99:SI
Failed to match this instruction:
(set (reg:SI 100)
    (and:SI (lshiftrt:SI (reg:SI 103)
            (const_int 8 [0x8]))
        (const_int 6 [0x6])))

That should match already, perhaps with a splitter.  aarch64 does not
have very generic rotate-and-mask (or -insert) instructions, so the
aarch64 backend needs to help combine with the less trivial cases.

If you have a splitter for *this* one, all else will probably work
"automatically": you split it to two ubfm, and the second of those can
then merge into the compare instruction, and everything works out.

> Also, this issue is there for ports like x86.  If we go that way, then we need to handle each port affected.  

Yes, you need to do target-specific stuff in every backend separately.

> So I am inclined to handle this in an arch-independent way.  

But you don't.  The transformation you do looks to be actively harmful
on many targets.  (I haven't tested it yet, that takes 8h currently).

> > What should be tested is what new combinations are done, and which are *no
> > longer* done.
> 
> In theory, we won't lose but emit more zero_extract with my patch.  

Which is a bad idea.  How many machine insns are there matters; the cost
of those matters (assuming that matches reality well, which it has to
anyway); latency matters.  How many insns are zero_extract instead of
something else is not a good indicator of performance.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Tuesday, March 17, 2020 1:58 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> On Mon, Mar 16, 2020 at 06:29:39AM +0000, Yangfei (Felix) wrote:
> > Sorry for not getting your point here.
> 
> Trying 7 -> 8:
>     7: r99:SI=r103:SI>>0x8
>       REG_DEAD r103:SI
>     8: r100:SI=r99:SI&0x6
>       REG_DEAD r99:SI
> Failed to match this instruction:
> (set (reg:SI 100)
>     (and:SI (lshiftrt:SI (reg:SI 103)
>             (const_int 8 [0x8]))
>         (const_int 6 [0x6])))
> 
> That should match already, perhaps with a splitter.  aarch64 does not have
> very generic rotate-and-mask (or -insert) instructions, so the
> aarch64 backend needs to help combine with the less trivial cases.
> 
> If you have a splitter for *this* one, all else will probably work
> "automatically": you split it to two ubfm, and the second of those can then
> merge into the compare instruction, and everything works out.

Do you mean splitting the above pattern into a combination of ubfx and ubfiz?  (Both are aliases of ubfm).  
I still don't see how the benefit can be achieved.  
The following is the expected assembly for the test case:  
        tst     x0, 1536
        cset    w0, ne
        ret
This may not happen when the remaining ubfx is there.  Also what instruction be matched when ubfiz is merged into the compare?  
Anything I missed?  

> > Also, this issue is there for ports like x86.  If we go that way, then we need
> to handle each port affected.
> 
> Yes, you need to do target-specific stuff in every backend separately.
> 
> > So I am inclined to handle this in an arch-independent way.
> 
> But you don't.  The transformation you do looks to be actively harmful on
> many targets.  (I haven't tested it yet, that takes 8h currently).

Now I know your concern about zero_extract.  Maybe this should be mentioned in docs like gccint.  
Also it's interesting to see how this may affect on those archs.  

Thanks,
Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

Hi!

On Tue, Mar 17, 2020 at 02:05:19AM +0000, Yangfei (Felix) wrote:
> > Trying 7 -> 8:
> >     7: r99:SI=r103:SI>>0x8
> >       REG_DEAD r103:SI
> >     8: r100:SI=r99:SI&0x6
> >       REG_DEAD r99:SI
> > Failed to match this instruction:
> > (set (reg:SI 100)
> >     (and:SI (lshiftrt:SI (reg:SI 103)
> >             (const_int 8 [0x8]))
> >         (const_int 6 [0x6])))
> > 
> > That should match already, perhaps with a splitter.  aarch64 does not have
> > very generic rotate-and-mask (or -insert) instructions, so the
> > aarch64 backend needs to help combine with the less trivial cases.
> > 
> > If you have a splitter for *this* one, all else will probably work
> > "automatically": you split it to two ubfm, and the second of those can then
> > merge into the compare instruction, and everything works out.
> 
> Do you mean splitting the above pattern into a combination of ubfx and ubfiz?  (Both are aliases of ubfm).  

Sure.  The problem with aarch's bitfield instruction is that either the
source or the dest has to be right-aligned, which isn't natural for the
compiler.

> I still don't see how the benefit can be achieved.  
> The following is the expected assembly for the test case:  
>         tst     x0, 1536
>         cset    w0, ne
>         ret
> This may not happen when the remaining ubfx is there.  Also what instruction be matched when ubfiz is merged into the compare?  
> Anything I missed?  

The second insn could combine with the compare, and then that can combine
back further.

Another approach:

Trying 7 -> 9:
    7: r99:SI=r103:SI>>0x8
      REG_DEAD r103:SI
    9: cc:CC_NZ=cmp(r99:SI&0x6,0)
      REG_DEAD r99:SI
Failed to match this instruction:
(set (reg:CC_NZ 66 cc)
    (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 103)
                (const_int 8 [0x8]))
            (const_int 6 [0x6]))
        (const_int 0 [0])))

This can be recognised as just that "tst" insn, no?  But combine (or
simplify-rtx) should get rid of the shift here, just the "and" is
simpler after all (it just needs to change the constant for that).

> Also it's interesting to see how this may affect on those archs.  

Yes.  Which is why "canonicalisation" rules that are really just "this
works better on targets A and B" do not usually work well.  Rules that
are predictable and that actually simplify the code might still need
all targets to update (and target maintainers will grumble, myself
included), but at least that is a way forwards (and not backwards or
sideways).


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Thursday, March 19, 2020 7:52 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> Hi!
> 
> On Tue, Mar 17, 2020 at 02:05:19AM +0000, Yangfei (Felix) wrote:
> > > Trying 7 -> 8:
> > >     7: r99:SI=r103:SI>>0x8
> > >       REG_DEAD r103:SI
> > >     8: r100:SI=r99:SI&0x6
> > >       REG_DEAD r99:SI
> > > Failed to match this instruction:
> > > (set (reg:SI 100)
> > >     (and:SI (lshiftrt:SI (reg:SI 103)
> > >             (const_int 8 [0x8]))
> > >         (const_int 6 [0x6])))
> > >
> > > That should match already, perhaps with a splitter.  aarch64 does
> > > not have very generic rotate-and-mask (or -insert) instructions, so
> > > the
> > > aarch64 backend needs to help combine with the less trivial cases.
> > >
> > > If you have a splitter for *this* one, all else will probably work
> > > "automatically": you split it to two ubfm, and the second of those
> > > can then merge into the compare instruction, and everything works out.
> >
> > Do you mean splitting the above pattern into a combination of ubfx and ubfiz?
> (Both are aliases of ubfm).
> 
> Sure.  The problem with aarch's bitfield instruction is that either the source or
> the dest has to be right-aligned, which isn't natural for the compiler.
> 
> > I still don't see how the benefit can be achieved.
> > The following is the expected assembly for the test case:
> >         tst     x0, 1536
> >         cset    w0, ne
> >         ret
> > This may not happen when the remaining ubfx is there.  Also what
> instruction be matched when ubfiz is merged into the compare?
> > Anything I missed?
> 
> The second insn could combine with the compare, and then that can combine
> back further.

Let me paste the RTL input to the combine phase:
/************************
(insn 6 3 7 2 (set (reg:SI 98)
        (ashiftrt:SI (reg:SI 102)
            (const_int 8 [0x8]))) "foo.c":3:16 742 {*aarch64_ashr_sisd_or_int_si3}
     (expr_list:REG_DEAD (reg:SI 102)
        (nil)))
(note 7 6 8 2 NOTE_INSN_DELETED)
(insn 8 7 9 2 (set (reg:CC_NZ 66 cc)
        (compare:CC_NZ (and:SI (reg:SI 98)
                (const_int 6 [0x6]))
            (const_int 0 [0]))) "foo.c":5:8 698 {*andsi3nr_compare0}
     (expr_list:REG_DEAD (reg:SI 98)
        (nil)))
(note 9 8 14 2 NOTE_INSN_DELETED)
(insn 14 9 15 2 (set (reg/i:SI 0 x0)
        (ne:SI (reg:CC_NZ 66 cc)
            (const_int 0 [0]))) "foo.c":10:1 494 {aarch64_cstoresi}
     (expr_list:REG_DEAD (reg:CC 66 cc)
        (nil)))
*************************/

Two issues that I can see here:
1. When the ubfiz is combined with the compare, the combined insn does not necessarily mean a equality comparison with zero.  
  This is also the case when all the three insns (ubfx & ubfiz & compare) are combined together.  

2. Given that the patterns for ubfx and ubfiz are already not simple, I am afraid the pattern we got by combining the three would be much complex.
  And even more complex when further merged with insn 14 here in order to make sure that we are doing a equality comparison with zero.  

So it looks difficult when we go this port-specific way without matching a "zero_extact".  

> Another approach:
> 
> Trying 7 -> 9:
>     7: r99:SI=r103:SI>>0x8
>       REG_DEAD r103:SI
>     9: cc:CC_NZ=cmp(r99:SI&0x6,0)
>       REG_DEAD r99:SI
> Failed to match this instruction:
> (set (reg:CC_NZ 66 cc)
>     (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 103)
>                 (const_int 8 [0x8]))
>             (const_int 6 [0x6]))
>         (const_int 0 [0])))
> 
> This can be recognised as just that "tst" insn, no?  But combine (or
> simplify-rtx) should get rid of the shift here, just the "and" is simpler after all (it
> just needs to change the constant for that).

No, this does not mean an equality comparison with zero.  I have mentioned this in my previous mail.  

Thanks,
Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Thu, Mar 19, 2020 at 01:43:40AM +0000, Yangfei (Felix) wrote:
> 2. Given that the patterns for ubfx and ubfiz are already not simple, I am afraid the pattern we got by combining the three would be much complex.
>   And even more complex when further merged with insn 14 here in order to make sure that we are doing a equality comparison with zero.  

It will be just as simple as with the other approach:

> > Another approach:
> > 
> > Trying 7 -> 9:
> >     7: r99:SI=r103:SI>>0x8
> >       REG_DEAD r103:SI
> >     9: cc:CC_NZ=cmp(r99:SI&0x6,0)
> >       REG_DEAD r99:SI
> > Failed to match this instruction:
> > (set (reg:CC_NZ 66 cc)
> >     (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 103)
> >                 (const_int 8 [0x8]))
> >             (const_int 6 [0x6]))
> >         (const_int 0 [0])))
> > 
> > This can be recognised as just that "tst" insn, no?  But combine (or
> > simplify-rtx) should get rid of the shift here, just the "and" is simpler after all (it
> > just needs to change the constant for that).
> 
> No, this does not mean an equality comparison with zero.  I have mentioned this in my previous mail.  

This should be simplified to
(set (reg:CC_NZ 66 cc)
     (compare:CC_NZ (and:SI (reg:SI 103)
                            (const_int 1536))
                    (const_int 0)))
(but it isn't), and that is just *and<mode>3nr_compare0, which is a
"tst" instruction.  If this is fixed (in simplify-rtx.c), it will work
as you want.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Friday, March 20, 2020 9:38 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> On Thu, Mar 19, 2020 at 01:43:40AM +0000, Yangfei (Felix) wrote:
> > 2. Given that the patterns for ubfx and ubfiz are already not simple, I am
> afraid the pattern we got by combining the three would be much complex.
> >   And even more complex when further merged with insn 14 here in order to
> make sure that we are doing a equality comparison with zero.
> 
> It will be just as simple as with the other approach:

I think the problem here is how to make sure we are doing a ***equality*** comparison with zero.  
We can only do the transformation under this condition.  
Then I see combine tries the following pattern: 

173 Failed to match this instruction:
174 (set (reg:SI 101)
175     (ne:SI (and:SI (lshiftrt:SI (reg:SI 102)
176                 (const_int 8 [0x8]))
177             (const_int 6 [0x6]))
178         (const_int 0 [0])))

But this cannot match a 'tst' instruction as the above pattern does not clobber the CC flag register.  
Also this means that we depend on specific uses cases, so may need different patterns to match all possible cases.  

> > > Another approach:
> > >
> > > Trying 7 -> 9:
> > >     7: r99:SI=r103:SI>>0x8
> > >       REG_DEAD r103:SI
> > >     9: cc:CC_NZ=cmp(r99:SI&0x6,0)
> > >       REG_DEAD r99:SI
> > > Failed to match this instruction:
> > > (set (reg:CC_NZ 66 cc)
> > >     (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 103)
> > >                 (const_int 8 [0x8]))
> > >             (const_int 6 [0x6]))
> > >         (const_int 0 [0])))
> > >
> > > This can be recognised as just that "tst" insn, no?  But combine (or
> > > simplify-rtx) should get rid of the shift here, just the "and" is
> > > simpler after all (it just needs to change the constant for that).
> >
> > No, this does not mean an equality comparison with zero.  I have mentioned
> this in my previous mail.
> 
> This should be simplified to
> (set (reg:CC_NZ 66 cc)
>      (compare:CC_NZ (and:SI (reg:SI 103)
>                             (const_int 1536))
>                     (const_int 0)))
> (but it isn't), and that is just *and<mode>3nr_compare0, which is a "tst"
> instruction.  If this is fixed (in simplify-rtx.c), it will work as you want.

But I don't think it's correct for logic in simplify-rtx.c to further simplify this rtl:  

(compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 102)
            (const_int 8 [0x8]))
        (const_int 6 [0x6]))
    (const_int 0 [0]))

The reason is that it knows nothing about CC_NZ.  
CC_NZ is aarch64-port specific and it does not necessarily mean a equality comparison with zero.  
Correct me if I missed anything.  

Thanks,
Felix

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Mon, Mar 23, 2020 at 07:46:17AM +0000, Yangfei (Felix) wrote:
> I think the problem here is how to make sure we are doing a ***equality*** comparison with zero.  
> We can only do the transformation under this condition.  
> Then I see combine tries the following pattern: 
> 
> 173 Failed to match this instruction:
> 174 (set (reg:SI 101)
> 175     (ne:SI (and:SI (lshiftrt:SI (reg:SI 102)
> 176                 (const_int 8 [0x8]))
> 177             (const_int 6 [0x6]))
> 178         (const_int 0 [0])))

Like I said, this should be simplified to
(set (reg:SI 10)
     (ne:SI (and:SI (reg:SI 102)
                    (const_int 1536))
            (const_int 0)))

> But this cannot match a 'tst' instruction as the above pattern does not clobber the CC flag register.  

combine can (and will) add such clobbers itself, if the pattern matches
otherwise, so that is not an issue.

> > This should be simplified to
> > (set (reg:CC_NZ 66 cc)
> >      (compare:CC_NZ (and:SI (reg:SI 103)
> >                             (const_int 1536))
> >                     (const_int 0)))
> > (but it isn't), and that is just *and<mode>3nr_compare0, which is a "tst"
> > instruction.  If this is fixed (in simplify-rtx.c), it will work as you want.
> 
> But I don't think it's correct for logic in simplify-rtx.c to further simplify this rtl:  
> 
> (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 102)
>             (const_int 8 [0x8]))
>         (const_int 6 [0x6]))
>     (const_int 0 [0]))

> The reason is that it knows nothing about CC_NZ.  

Yeah, maybe not in simplify-rtx.c, hrm.  There is SELECT_CC_MODE for
these things, and combine knows about that (not many passes do).


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi!

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Monday, March 23, 2020 8:10 PM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> Yeah, maybe not in simplify-rtx.c, hrm.  There is SELECT_CC_MODE for these
> things, and combine knows about that (not many passes do).

I modified combine emitting a simple AND operation instead of making one zero_extract for this scenario.  
Attached please find the new patch.  Hope this solves both of our concerns.  

gcc
+2020-03-24  Felix Yang  <felix.yang@huawei.com> 
+
+       PR rtl-optimization/94026
+       * combine.c (make_compound_operation_int): If we are have (and
+       (lshiftrt X C) M) and M is a constant that would select a field
+       of bits within an item, but not the entire word, fold this into
+       a simple AND if we are in an equality comparison.

gcc/testsuite
+2020-03-24  Felix Yang  <felix.yang@huawei.com> 
+
+       PR rtl-optimization/94026
+       * gcc.dg/pr94026.c: New test.


Thanks,
Felix

[-- Attachment #2: pr94026-v1.patch --]
[-- Type: application/octet-stream, Size: 2322 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index 58366a6..10c332d 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8170,6 +8170,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
 	 is a logical right shift, make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
@@ -8223,6 +8227,23 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	  new_rtx = make_compound_operation (new_rtx, in_code);
 	}
 
+      /* If we are have (and (lshiftrt X C) M) and M is a constant that would
+	 select a field of bits within an item, but not the entire word, this
+	 might be representable by a simple AND if we are in an equality
+	 comparison.  */
+      else if (pos > 0 && equality_comparison
+	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
+	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
+	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
+		  <= GET_MODE_BITSIZE (mode))
+	{
+	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
+	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 << len) - 1;
+	  new_rtx = gen_rtx_AND (mode, new_rtx,
+				 gen_int_mode (mask << real_pos, mode));
+	}
+
       /* If we are have (and (rotate X C) M) and C is larger than the number
 	 of bits in M, this is an extraction.  */
 
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 0000000..5f309b7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,21 @@
+/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fdump-rtl-combine" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
+   RTL pattern, regardless of whether an actual insn may be found on the
+   platform.  */
+
+/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Tue, Mar 24, 2020 at 06:30:12AM +0000, Yangfei (Felix) wrote:
> I modified combine emitting a simple AND operation instead of making one zero_extract for this scenario.  
> Attached please find the new patch.  Hope this solves both of our concerns.  

This looks promising.  I'll try it out, see what it does on other
targets.  (It will have to wait for GCC 11 stage 1, of course).

Thanks!


Segher


p.s.  Please use a correct mime type?  application/octet-stream isn't
something I can reply to.  Just text/plain is fine :-)

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Tuesday, March 24, 2020 10:58 PM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> On Tue, Mar 24, 2020 at 06:30:12AM +0000, Yangfei (Felix) wrote:
> > I modified combine emitting a simple AND operation instead of making one
> zero_extract for this scenario.
> > Attached please find the new patch.  Hope this solves both of our concerns.
> 
> This looks promising.  I'll try it out, see what it does on other targets.  (It will
> have to wait for GCC 11 stage 1, of course).

I see GCC11 stage 1 opens for commits now.
I have rebased the patch on the latest repo.  Attached please find the v2 patch.
Bootstrapped and tested on x86-64-linux-gnu and aarch64-linux-gnu.
Is this good to go?
 
> 
> p.s.  Please use a correct mime type?  application/octet-stream isn't
> something I can reply to.  Just text/plain is fine :-)

I have using plain text now, hope that works for you.  :-)

Thanks,
Felix

[-- Attachment #2: pr94026-v2.diff --]
[-- Type: application/octet-stream, Size: 4448 bytes --]

From bf4ef7ada5c3687c695ff6b3738ae6f943521b75 Mon Sep 17 00:00:00 2001
From: Fei Yang <felix.yang@huawei.com>
Date: Wed, 6 May 2020 16:33:40 +0800
Subject: [PATCH] combine: missed opportunity to simplify comparisons with zero
 [PR94026]

If we have (and (lshiftrt X C) M) and M is a constant that would select
a field of bits within an item, but not the entire word, fold this into
a simple AND if we are in an equality comparison against zero.

2020-05-06  Felix Yang  <felix.yang@huawei.com>

gcc/
    PR rtl-optimization/94026
    * combine.c (make_compound_operation_int): If we have (and
    (lshiftrt X C) M) and M is a constant that would select a field
    of bits within an item, but not the entire word, fold this into
    a simple AND if we are in an equality comparison.

gcc/testsuite/
    PR rtl-optimization/94026
    * gcc.dg/pr94026.c: New test.
---
 gcc/ChangeLog                  |  8 ++++++++
 gcc/combine.c                  | 20 ++++++++++++++++++++
 gcc/testsuite/ChangeLog        |  5 +++++
 gcc/testsuite/gcc.dg/pr94026.c | 21 +++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr94026.c

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 15ac1c3c4df..58c78dcdb3a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2020-05-06  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* combine.c (make_compound_operation_int): If we have (and
+	(lshiftrt X C) M) and M is a constant that would select a field
+	of bits within an item, but not the entire word, fold this into
+	a simple AND if we are in an equality comparison.
+
 2020-05-05  Michael Meissner  <meissner@linux.ibm.com>
 
 	* config/rs6000/rs6000-builtin.def: Delete changes meant for a
diff --git a/gcc/combine.c b/gcc/combine.c
index 4c324f38660..351aea9dfed 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8177,6 +8177,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
 	 is a logical right shift, make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
@@ -8230,6 +8234,22 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	  new_rtx = make_compound_operation (new_rtx, in_code);
 	}
 
+      /* If we have (and (lshiftrt X C) M) and M is a constant that would select
+	 a field of bits within an item, but not the entire word, this might be
+	 representable by a simple AND if we are in an equality comparison.  */
+      else if (pos > 0 && equality_comparison
+	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
+	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
+	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
+		  <= GET_MODE_BITSIZE (mode))
+	{
+	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
+	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 << len) - 1;
+	  new_rtx = gen_rtx_AND (mode, new_rtx,
+				 gen_int_mode (mask << real_pos, mode));
+	}
+
       /* If we are have (and (rotate X C) M) and C is larger than the number
 	 of bits in M, this is an extraction.  */
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 40a925e87ea..1c865a1b1ab 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-06  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* gcc.dg/pr94026.c: New test.
+
 2020-05-05  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR testsuite/84324
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 00000000000..5f309b7a34e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,21 @@
+/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fdump-rtl-combine" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
+   RTL pattern, regardless of whether an actual insn may be found on the
+   platform.  */
+
+/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */
-- 
2.19.1

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

Hi!

On Wed, May 06, 2020 at 08:57:52AM +0000, Yangfei (Felix) wrote:
> > This looks promising.  I'll try it out, see what it does on other targets.  (It will
> > have to wait for GCC 11 stage 1, of course).
> 
> I see GCC11 stage 1 opens for commits now.
> I have rebased the patch on the latest repo.  Attached please find the v2 patch.
> Bootstrapped and tested on x86-64-linux-gnu and aarch64-linux-gnu.
> Is this good to go?

I'll try it out.  Takes a day or so...  Stay tuned.

> > p.s.  Please use a correct mime type?  application/octet-stream isn't
> > something I can reply to.  Just text/plain is fine :-)
> 
> I have using plain text now, hope that works for you.  :-)

It is still application/octet-stream.


Segher

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

Hi!

Sorry this is taking so long.

On Wed, May 06, 2020 at 08:57:52AM +0000, Yangfei (Felix) wrote:
> > On Tue, Mar 24, 2020 at 06:30:12AM +0000, Yangfei (Felix) wrote:
> > > I modified combine emitting a simple AND operation instead of making one
> > zero_extract for this scenario.
> > > Attached please find the new patch.  Hope this solves both of our concerns.
> > 
> > This looks promising.  I'll try it out, see what it does on other targets.  (It will
> > have to wait for GCC 11 stage 1, of course).

It creates better code on all targets :-)  A quite small improvement, but
not entirely trivial.

> > p.s.  Please use a correct mime type?  application/octet-stream isn't
> > something I can reply to.  Just text/plain is fine :-)
> 
> I have using plain text now, hope that works for you.  :-)

Nope:

[-- Attachment #2: pr94026-v2.diff --]
[-- Type: application/octet-stream, Encoding: base64, Size: 5.9K --]


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Saturday, May 23, 2020 10:57 PM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero
> 
> Hi!
> 
> Sorry this is taking so long.
> 
> On Wed, May 06, 2020 at 08:57:52AM +0000, Yangfei (Felix) wrote:
> > > On Tue, Mar 24, 2020 at 06:30:12AM +0000, Yangfei (Felix) wrote:
> > > > I modified combine emitting a simple AND operation instead of
> > > > making one
> > > zero_extract for this scenario.
> > > > Attached please find the new patch.  Hope this solves both of our
> concerns.
> > >
> > > This looks promising.  I'll try it out, see what it does on other
> > > targets.  (It will have to wait for GCC 11 stage 1, of course).
> 
> It creates better code on all targets :-)  A quite small improvement, but not
> entirely trivial.

Thanks for the effort.  It's great to hear that :- )
Attached please find the v3 patch.  Rebased on the latest trunk. 
Bootstrapped and tested on aarch64-linux-gnu.  Could you please help install it?

> > > p.s.  Please use a correct mime type?  application/octet-stream
> > > isn't something I can reply to.  Just text/plain is fine :-)
> >
> > I have using plain text now, hope that works for you.  :-)
> 
> Nope:
> 
> [-- Attachment #2: pr94026-v2.diff --]
> [-- Type: application/octet-stream, Encoding: base64, Size: 5.9K --]

This time I switched to use UUEncode type for the attachment.  Does it work?
I am using Outlook and I didn't find the place to change the MIME type : - (

Felix

[-- Attachment #2: pr94026-v3.diff --]
[-- Type: application/octet-stream, Size: 4425 bytes --]

From a444419238c02c1e6ab9593a14a13e1e3dff90ed Mon Sep 17 00:00:00 2001
From: Fei Yang <felix.yang@huawei.com>
Date: Mon, 25 May 2020 10:19:30 +0800
Subject: [PATCH] combine: missed opportunity to simplify comparisons with zero
 [PR94026]

If we have (and (lshiftrt X C) M) and M is a constant that would select
a field of bits within an item, but not the entire word, fold this into
a simple AND if we are in an equality comparison against zero.

2020-05-25  Felix Yang  <felix.yang@huawei.com>

gcc/
    PR rtl-optimization/94026
    * combine.c (make_compound_operation_int): If we have (and
    (lshiftrt X C) M) and M is a constant that would select a field
    of bits within an item, but not the entire word, fold this into
    a simple AND if we are in an equality comparison.

gcc/testsuite/
    PR rtl-optimization/94026
    * gcc.dg/pr94026.c: New test.
---
 gcc/ChangeLog                  |  8 ++++++++
 gcc/combine.c                  | 20 ++++++++++++++++++++
 gcc/testsuite/ChangeLog        |  5 +++++
 gcc/testsuite/gcc.dg/pr94026.c | 21 +++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr94026.c

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ede1f15eb7a..a2637015c5b 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2020-05-25  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* combine.c (make_compound_operation_int): If we have (and
+	(lshiftrt X C) M) and M is a constant that would select a field
+	of bits within an item, but not the entire word, fold this into
+	a simple AND if we are in an equality comparison.
+
 2020-05-23 Iain Sandoe <iain@sandoe.co.uk>
 
 	* config/darwin.h (ASM_GENERATE_INTERNAL_LABEL): Make
diff --git a/gcc/combine.c b/gcc/combine.c
index b044f29fd36..76d62b0bd17 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8178,6 +8178,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
 	 is a logical right shift, make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
@@ -8231,6 +8235,22 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	  new_rtx = make_compound_operation (new_rtx, in_code);
 	}
 
+      /* If we have (and (lshiftrt X C) M) and M is a constant that would select
+	 a field of bits within an item, but not the entire word, this might be
+	 representable by a simple AND if we are in an equality comparison.  */
+      else if (pos > 0 && equality_comparison
+	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
+	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
+	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
+		  <= GET_MODE_BITSIZE (mode))
+	{
+	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
+	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 << len) - 1;
+	  new_rtx = gen_rtx_AND (mode, new_rtx,
+				 gen_int_mode (mask << real_pos, mode));
+	}
+
       /* If we are have (and (rotate X C) M) and C is larger than the number
 	 of bits in M, this is an extraction.  */
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 2710d2ee490..20a41f8072e 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-25  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* gcc.dg/pr94026.c: New test.
+
 2020-05-24  Harald Anlauf  <anlauf@gmx.de>
 
 	PR fortran/95106
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 00000000000..5f309b7a34e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,21 @@
+/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fdump-rtl-combine" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
+   RTL pattern, regardless of whether an actual insn may be found on the
+   platform.  */
+
+/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */
-- 
2.19.1

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

On Mon, May 25, 2020 at 02:59:30AM +0000, Yangfei (Felix) wrote:
> > It creates better code on all targets :-)  A quite small improvement, but not
> > entirely trivial.
> 
> Thanks for the effort.  It's great to hear that :- )

Yes :-)

> > > > p.s.  Please use a correct mime type?  application/octet-stream
> > > > isn't something I can reply to.  Just text/plain is fine :-)
> > >
> > > I have using plain text now, hope that works for you.  :-)
> > 
> > Nope:
> > 
> > [-- Attachment #2: pr94026-v2.diff --]
> > [-- Type: application/octet-stream, Encoding: base64, Size: 5.9K --]
> 
> This time I switched to use UUEncode type for the attachment.  Does it work?

No:

[-- Attachment #2: pr94026-v3.diff --]
[-- Type: application/octet-stream, Encoding: base64, Size: 5.8K --]

> I am using Outlook and I didn't find the place to change the MIME type : - (

The simplest option is to use a different email client, one that plays
nicely with others.  You use git, maybe you could even use git-send-email?

I'll paste things manually...

> From a444419238c02c1e6ab9593a14a13e1e3dff90ed Mon Sep 17 00:00:00 2001
> From: Fei Yang <felix.yang@huawei.com>
> Date: Mon, 25 May 2020 10:19:30 +0800
> Subject: [PATCH] combine: missed opportunity to simplify comparisons with zero
>  [PR94026]

(Capital "M" on "Missed" please)

But, the subject should say what the patch *does*.  So maybe
  combine: Simplify more comparisons with zero (PR94026)

> If we have (and (lshiftrt X C) M) and M is a constant that would select
> a field of bits within an item, but not the entire word, fold this into
> a simple AND if we are in an equality comparison against zero.

But that subject doesn't really describe what the patch does, anyway?

> gcc/
>     PR rtl-optimization/94026
>     * combine.c (make_compound_operation_int): If we have (and
>     (lshiftrt X C) M) and M is a constant that would select a field
>     of bits within an item, but not the entire word, fold this into
>     a simple AND if we are in an equality comparison.
> 
> gcc/testsuite/
>     PR rtl-optimization/94026
>     * gcc.dg/pr94026.c: New test.

> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,11 @@
> +2020-05-25  Felix Yang  <felix.yang@huawei.com>
> +
> +	PR rtl-optimization/94026
> +	* combine.c (make_compound_operation_int): If we have (and
> +	(lshiftrt X C) M) and M is a constant that would select a field
> +	of bits within an item, but not the entire word, fold this into
> +	a simple AND if we are in an equality comparison.

Don't put the changelog in the patch.

> diff --git a/gcc/combine.c b/gcc/combine.c
> index b044f29fd36..76d62b0bd17 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -8178,6 +8178,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
>        if (!CONST_INT_P (XEXP (x, 1)))
>  	break;
>  
> +      HOST_WIDE_INT pos;
> +      unsigned HOST_WIDE_INT len;
> +      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);

      unsigned HOST_WIDE_INT len;
      HOST_WIDE_INT pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);

> @@ -8231,6 +8235,22 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
>  	  new_rtx = make_compound_operation (new_rtx, in_code);
>  	}
>  
> +      /* If we have (and (lshiftrt X C) M) and M is a constant that would select
> +	 a field of bits within an item, but not the entire word, this might be
> +	 representable by a simple AND if we are in an equality comparison.  */
> +      else if (pos > 0 && equality_comparison

That "&& equality_comparison" should be on a separate line as well.

> +	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
> +	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> +	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
> +		  <= GET_MODE_BITSIZE (mode))
> +	{
> +	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
> +	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
> +	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 << len) - 1;

Space after cast.

> +	  new_rtx = gen_rtx_AND (mode, new_rtx,
> +				 gen_int_mode (mask << real_pos, mode));
> +	}

So this changes
  ((X >> C) & M) == ...
to
  (X & (M << C)) == ...
?

Where then does it check what ... is?  This is only valid like this if
that is zero.

Why should this go in combine and not in simplify-rtx instead?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94026.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */

Why restrict this to only some targets?

> +/* { dg-options "-O2 -fdump-rtl-combine" } */
> +
> +int
> +foo (int c)
> +{
> +  int a = (c >> 8) & 7;
> +
> +  if (a >= 2) {
> +    return 1;
> +  }
> +
> +  return 0;
> +}
> +
> +/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
> +   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
> +   RTL pattern, regardless of whether an actual insn may be found on the
> +   platform.  */
> +
> +/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */

That is a very fragile test.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Tuesday, May 26, 2020 12:27 AM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero

Snip...

> > I am using Outlook and I didn't find the place to change the MIME type
> > : - (
> 
> The simplest option is to use a different email client, one that plays nicely
> with others.  You use git, maybe you could even use git-send-email?

The bad news is that it would be hard to switch to a different email client with my company's IT policy  :-( 
But I think I can ask IT if that is possible. Sorry for the trouble.

> I'll paste things manually...
> 
> > From a444419238c02c1e6ab9593a14a13e1e3dff90ed Mon Sep 17 00:00:00
> 2001
> > From: Fei Yang <felix.yang@huawei.com>
> > Date: Mon, 25 May 2020 10:19:30 +0800
> > Subject: [PATCH] combine: missed opportunity to simplify comparisons
> > with zero  [PR94026]
> 
> (Capital "M" on "Missed" please)
> 
> But, the subject should say what the patch *does*.  So maybe
>   combine: Simplify more comparisons with zero (PR94026)

OK. 

> > If we have (and (lshiftrt X C) M) and M is a constant that would
> > select a field of bits within an item, but not the entire word, fold
> > this into a simple AND if we are in an equality comparison against zero.
> 
> But that subject doesn't really describe what the patch does, anyway?

OK.  Modified in the v4 patch.  Does it look better?

> > gcc/
> >     PR rtl-optimization/94026
> >     * combine.c (make_compound_operation_int): If we have (and
> >     (lshiftrt X C) M) and M is a constant that would select a field
> >     of bits within an item, but not the entire word, fold this into
> >     a simple AND if we are in an equality comparison.
> >
> > gcc/testsuite/
> >     PR rtl-optimization/94026
> >     * gcc.dg/pr94026.c: New test.
> 
> > --- a/gcc/ChangeLog
> > +++ b/gcc/ChangeLog
> > @@ -1,3 +1,11 @@
> > +2020-05-25  Felix Yang  <felix.yang@huawei.com>
> > +
> > +	PR rtl-optimization/94026
> > +	* combine.c (make_compound_operation_int): If we have (and
> > +	(lshiftrt X C) M) and M is a constant that would select a field
> > +	of bits within an item, but not the entire word, fold this into
> > +	a simple AND if we are in an equality comparison.
> 
> Don't put the changelog in the patch.

OK.  I paste it here:

gcc/ChangeLog

+2020-05-26  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* combine.c (make_compound_operation_int): If we have (and
+	(lshiftrt X C) M) and M is a constant that would select a field
+	of bits within an item, but not the entire word, fold this into
+	a simple AND if we are in an equality comparison.

gcc/testsuite/ChangeLog

+2020-05-26  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* gcc.dg/pr94026.c: New test.

> > diff --git a/gcc/combine.c b/gcc/combine.c index
> > b044f29fd36..76d62b0bd17 100644
> > --- a/gcc/combine.c
> > +++ b/gcc/combine.c
> > @@ -8178,6 +8178,10 @@ make_compound_operation_int
> (scalar_int_mode mode, rtx *x_ptr,
> >        if (!CONST_INT_P (XEXP (x, 1)))
> >  	break;
> >
> > +      HOST_WIDE_INT pos;
> > +      unsigned HOST_WIDE_INT len;
> > +      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
> 
>       unsigned HOST_WIDE_INT len;
>       HOST_WIDE_INT pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
> 
> > @@ -8231,6 +8235,22 @@ make_compound_operation_int
> (scalar_int_mode mode, rtx *x_ptr,
> >  	  new_rtx = make_compound_operation (new_rtx, in_code);
> >  	}
> >
> > +      /* If we have (and (lshiftrt X C) M) and M is a constant that would
> select
> > +	 a field of bits within an item, but not the entire word, this might be
> > +	 representable by a simple AND if we are in an equality comparison.
> */
> > +      else if (pos > 0 && equality_comparison
> 
> That "&& equality_comparison" should be on a separate line as well.

OK.

> > +	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
> > +	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> > +	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
> > +		  <= GET_MODE_BITSIZE (mode))
> > +	{
> > +	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0),
> next_code);
> > +	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
> > +	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 <<
> len) -
> > +1;
> 
> Space after cast.

OK.

> > +	  new_rtx = gen_rtx_AND (mode, new_rtx,
> > +				 gen_int_mode (mask << real_pos, mode));
> > +	}
> 
> So this changes
>   ((X >> C) & M) == ...
> to
>   (X & (M << C)) == ...
> ?
> 
> Where then does it check what ... is?  This is only valid like this if that is zero.
> 
> Why should this go in combine and not in simplify-rtx instead?

True.  This is only valid when ... is zero.
That's why we need the "&& equality_comparison " condition here.

> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr94026.c
> > @@ -0,0 +1,21 @@
> > +/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
> 
> Why restrict this to only some targets?

That's because I only have these targets for verification.
But I think this can work on other targets.  Removed from the v4 patch.
Could you please help check the other ports?

> > +/* { dg-options "-O2 -fdump-rtl-combine" } */
> > +
> > +int
> > +foo (int c)
> > +{
> > +  int a = (c >> 8) & 7;
> > +
> > +  if (a >= 2) {
> > +    return 1;
> > +  }
> > +
> > +  return 0;
> > +}
> > +
> > +/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
> > +   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
> > +   RTL pattern, regardless of whether an actual insn may be found on the
> > +   platform.  */
> > +
> > +/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */
> 
> That is a very fragile test.

For this specific test case, (const_int 1536) is calculated from subexpression (M << C) in (X & (M << C)).
I also see some similar checkings in gcc.dg/asr_div1.c.  Suggesions?

Felix

[-- Attachment #2: pr94026-v4.diff --]
[-- Type: application/octet-stream, Size: 3370 bytes --]

From 91870dbc8cbf6e35d208d5b657990226754f0b2e Mon Sep 17 00:00:00 2001
From: Fei Yang <felix.yang@huawei.com>
Date: Tue, 26 May 2020 10:41:47 +0800
Subject: [PATCH] combine: Simplify more comparisons with zero [PR94026]

In rtl combine when we have an equality comparison of (and (lshiftrt X C) M)
and zero, simplify this into a simple AND if M is a constant that would select
a field of bits within an item, but not the entire word.

2020-05-26  Felix Yang  <felix.yang@huawei.com>

gcc/
    PR rtl-optimization/94026
    * combine.c (make_compound_operation_int): If we have (and
    (lshiftrt X C) M) and M is a constant that would select a field
    of bits within an item, but not the entire word, fold this into
    a simple AND if we are in an equality comparison.

gcc/testsuite/
    PR rtl-optimization/94026
    * gcc.dg/pr94026.c: New test.
---
 gcc/combine.c                  | 21 +++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr94026.c | 21 +++++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr94026.c

diff --git a/gcc/combine.c b/gcc/combine.c
index b044f29fd36..c5787267116 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8178,6 +8178,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
 	 is a logical right shift, make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
@@ -8231,6 +8235,23 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	  new_rtx = make_compound_operation (new_rtx, in_code);
 	}
 
+      /* If we have (and (lshiftrt X C) M) and M is a constant that would select
+	 a field of bits within an item, but not the entire word, this might be
+	 representable by a simple AND if we are in an equality comparison.  */
+      else if (pos > 0
+	       && equality_comparison
+	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
+	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
+	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
+		  <= GET_MODE_BITSIZE (mode))
+	{
+	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
+	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT) 1 << len) - 1;
+	  new_rtx = gen_rtx_AND (mode, new_rtx,
+				 gen_int_mode (mask << real_pos, mode));
+	}
+
       /* If we are have (and (rotate X C) M) and C is larger than the number
 	 of bits in M, this is an extraction.  */
 
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 00000000000..2ddd2b1458c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-combine" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
+   RTL pattern, regardless of whether an actual insn may be found on the
+   platform.  */
+
+/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */
-- 
2.19.1

Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Segher Boessenkool]

Hi!

On Tue, May 26, 2020 at 03:45:05AM +0000, Yangfei (Felix) wrote:
> > > I am using Outlook and I didn't find the place to change the MIME type
> > > : - (
> > 
> > The simplest option is to use a different email client, one that plays nicely
> > with others.  You use git, maybe you could even use git-send-email?
> 
> The bad news is that it would be hard to switch to a different email client with my company's IT policy  :-( 
> But I think I can ask IT if that is possible. Sorry for the trouble.

Yes, please try to get this sorted somehow.  Maybe you can ask other
people in your company that have this same problem?

> > > +	  new_rtx = gen_rtx_AND (mode, new_rtx,
> > > +				 gen_int_mode (mask << real_pos, mode));
> > > +	}
> > 
> > So this changes
> >   ((X >> C) & M) == ...
> > to
> >   (X & (M << C)) == ...
> > ?
> > 
> > Where then does it check what ... is?  This is only valid like this if that is zero.
> > 
> > Why should this go in combine and not in simplify-rtx instead?
> 
> True.  This is only valid when ... is zero.
> That's why we need the "&& equality_comparison " condition here.

But that doesn't test if the other side of the comparison is 0.

> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/pr94026.c
> > > @@ -0,0 +1,21 @@
> > > +/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */
> > 
> > Why restrict this to only some targets?
> 
> That's because I only have these targets for verification.
> But I think this can work on other targets.  Removed from the v4 patch.
> Could you please help check the other ports?

In general, you should never restrict anything to some targets simply
because you haven't tested it on other targets.

If it is a good test it will just work on those other targets.  Traffic
on gcc-testresults@ will show you if it actually does.

> > > +/* { dg-options "-O2 -fdump-rtl-combine" } */
> > > +
> > > +int
> > > +foo (int c)
> > > +{
> > > +  int a = (c >> 8) & 7;
> > > +
> > > +  if (a >= 2) {
> > > +    return 1;
> > > +  }
> > > +
> > > +  return 0;
> > > +}
> > > +
> > > +/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
> > > +   to (compare (and (x 1536)) 0). We look for the *attempt* to match this
> > > +   RTL pattern, regardless of whether an actual insn may be found on the
> > > +   platform.  */
> > > +
> > > +/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */
> > 
> > That is a very fragile test.
> 
> For this specific test case, (const_int 1536) is calculated from subexpression (M << C) in (X & (M << C)).
> I also see some similar checkings in gcc.dg/asr_div1.c.  Suggesions?

Maybe it is better to test that the non-optimal code you saw before is
not generated anymore?  That could be a target-specific test of course.
The advantage is that the test will not break for all kinds of unrelated
reasons (like this test) -- that simply does not scale with the number
of tests we have.


Segher

RE: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero

[Yangfei (Felix)]

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

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Tuesday, May 26, 2020 11:32 PM
> To: Yangfei (Felix) <felix.yang@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaijian@huawei.com>
> Subject: Re: [PATCH PR94026] combine missed opportunity to simplify
> comparisons with zero

Snip...

> 
> Yes, please try to get this sorted somehow.  Maybe you can ask other people
> in your company that have this same problem?

Will try and see.

> > > > +	  new_rtx = gen_rtx_AND (mode, new_rtx,
> > > > +				 gen_int_mode (mask << real_pos, mode));
> > > > +	}
> > >
> > > So this changes
> > >   ((X >> C) & M) == ...
> > > to
> > >   (X & (M << C)) == ...
> > > ?
> > >
> > > Where then does it check what ... is?  This is only valid like this if that is
> zero.
> > >
> > > Why should this go in combine and not in simplify-rtx instead?
> >
> > True.  This is only valid when ... is zero.
> > That's why we need the "&& equality_comparison " condition here.
> 
> But that doesn't test if the other side of the comparison is 0.

Well, the caller has ensured that.

Here, local variable "equality_comparison" in make_compound_operation_int depends on parameter "in_code":
 8088   if (in_code == EQ)
 8089     {
 8090       equality_comparison = true;
 8091       in_code = COMPARE;
 8092     }

The only caller of make_compound_operation_int is make_compound_operation.
The comment of the caller says something about " in_code ":

 8512    IN_CODE says what kind of expression we are processing.  Normally, it is
 8513    SET.  In a memory address it is MEM.  When processing the arguments of
 8514    a comparison or a COMPARE against zero, it is COMPARE, or EQ if more
 8515    precisely it is an equality comparison against zero.  */

For the given test case, we have a call trace of:
(gdb) bt
#0  make_compound_operation_int (mode=..., x_ptr=0xffffffffbd08, in_code=COMPARE, next_code_ptr=0xffffffffbd1c) at ../../gcc-git/gcc/combine.c:8248
#1  0x000000000208983c in make_compound_operation (x=0xffffb211c768, in_code=EQ) at ../../gcc-git/gcc/combine.c:8539
#2  0x00000000020970fc in simplify_comparison (code=NE, pop0=0xffffffffc1e8, pop1=0xffffffffc1e0) at ../../gcc-git/gcc/combine.c:13032
#3  0x0000000002084544 in simplify_set (x=0xffffb211c240) at ../../gcc-git/gcc/combine.c:6932
#4  0x0000000002082688 in combine_simplify_rtx (x=0xffffb211c240, op0_mode=E_VOIDmode, in_dest=0, in_cond=0) at ../../gcc-git/gcc/combine.c:6445
#5  0x000000000208025c in subst (x=0xffffb211c240, from=0xffffb211c138, to=0xffffb211c150, in_dest=0, in_cond=0, unique_copy=0)
    at ../../gcc-git/gcc/combine.c:5724
#6  0x0000000002079110 in try_combine (i3=0xffffb22cc3c0, i2=0xffffb22cc340, i1=0x0, i0=0x0, new_direct_jump_p=0xffffffffceb4,
    last_combined_insn=0xffffb22cc3c0) at ../../gcc-git/gcc/combine.c:3413
#7  0x0000000002073004 in combine_instructions (f=0xffffb211d038, nregs=103) at ../../gcc-git/gcc/combine.c:1305
#8  0x000000000209cc50 in rest_of_handle_combine () at ../../gcc-git/gcc/combine.c:15088

In simplify_comparison (combine.c:13032):

13028   rtx_code op0_mco_code = SET;
13029   if (op1 == const0_rtx)
13030     op0_mco_code = code == NE || code == EQ ? EQ : COMPARE;
13031
13032   op0 = make_compound_operation (op0, op0_mco_code);
13033   op1 = make_compound_operation (op1, SET);


> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.dg/pr94026.c
> > > > @@ -0,0 +1,21 @@
> > > > +/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } }
> > > > +*/
> > >
> > > Why restrict this to only some targets?
> >
> > That's because I only have these targets for verification.
> > But I think this can work on other targets.  Removed from the v4 patch.
> > Could you please help check the other ports?
> 
> In general, you should never restrict anything to some targets simply
> because you haven't tested it on other targets.
> 
> If it is a good test it will just work on those other targets.  Traffic on gcc-
> testresults@ will show you if it actually does.

OK.  Thanks for pointing this out  :-)

> > > > +/* { dg-options "-O2 -fdump-rtl-combine" } */
> > > > +
> > > > +int
> > > > +foo (int c)
> > > > +{
> > > > +  int a = (c >> 8) & 7;
> > > > +
> > > > +  if (a >= 2) {
> > > > +    return 1;
> > > > +  }
> > > > +
> > > > +  return 0;
> > > > +}
> > > > +
> > > > +/* The combine phase should transform (compare (and (lshiftrt x 8) 6)
> 0)
> > > > +   to (compare (and (x 1536)) 0). We look for the *attempt* to match
> this
> > > > +   RTL pattern, regardless of whether an actual insn may be found on
> the
> > > > +   platform.  */
> > > > +
> > > > +/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } }
> > > > +*/
> > >
> > > That is a very fragile test.
> >
> > For this specific test case, (const_int 1536) is calculated from subexpression
> (M << C) in (X & (M << C)).
> > I also see some similar checkings in gcc.dg/asr_div1.c.  Suggesions?
> 
> Maybe it is better to test that the non-optimal code you saw before is not
> generated anymore?  That could be a target-specific test of course.
> The advantage is that the test will not break for all kinds of unrelated reasons
> (like this test) -- that simply does not scale with the number of tests we have.

Good suggestion. 
The v5 patch checks for the redundant "asr" instruction on aarch64. 
Newly add test fail without  the fix and pass otherwise. 


gcc/ChangeLog

+2020-05-27  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* combine.c (make_compound_operation_int): If we have (and
+	(lshiftrt X C) M) and M is a constant that would select a field
+	of bits within an item, but not the entire word, fold this into
+	a simple AND if we are in an equality comparison.

gcc/testsuite/ChangeLog

+2020-05-27  Felix Yang  <felix.yang@huawei.com>
+
+	PR rtl-optimization/94026
+	* gcc.dg/pr94026.c: New test.

[-- Attachment #2: pr94026-v5.diff --]
[-- Type: application/octet-stream, Size: 3512 bytes --]

From 2acace1f6edb50a29e1de6c0de6265669ca28092 Mon Sep 17 00:00:00 2001
From: Fei Yang <felix.yang@huawei.com>
Date: Wed, 27 May 2020 10:46:32 +0800
Subject: [PATCH] combine: Simplify more comparisons with zero [PR94026]

In rtl combine when we have an equality comparison of (and (lshiftrt X C) M)
and zero, simplify this into a simple AND if M is a constant that would select
a field of bits within an item, but not the entire word.

2020-05-27  Felix Yang  <felix.yang@huawei.com>

gcc/
    PR rtl-optimization/94026
    * combine.c (make_compound_operation_int): If we have (and
    (lshiftrt X C) M) and M is a constant that would select a field
    of bits within an item, but not the entire word, fold this into
    a simple AND if we are in an equality comparison.

gcc/testsuite/
    PR rtl-optimization/94026
    * gcc.dg/pr94026.c: New test.
---
 gcc/combine.c                  | 21 +++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr94026.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr94026.c

diff --git a/gcc/combine.c b/gcc/combine.c
index b044f29fd36..c5787267116 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8178,6 +8178,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
       if (!CONST_INT_P (XEXP (x, 1)))
 	break;
 
+      HOST_WIDE_INT pos;
+      unsigned HOST_WIDE_INT len;
+      pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len);
+
       /* If the constant is a power of two minus one and the first operand
 	 is a logical right shift, make an extraction.  */
       if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
@@ -8231,6 +8235,23 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr,
 	  new_rtx = make_compound_operation (new_rtx, in_code);
 	}
 
+      /* If we have (and (lshiftrt X C) M) and M is a constant that would select
+	 a field of bits within an item, but not the entire word, this might be
+	 representable by a simple AND if we are in an equality comparison.  */
+      else if (pos > 0
+	       && equality_comparison
+	       && GET_CODE (XEXP (x, 0)) == LSHIFTRT
+	       && CONST_INT_P (XEXP (XEXP (x, 0), 1))
+	       && pos + UINTVAL (XEXP (XEXP (x, 0), 1))
+		  <= GET_MODE_BITSIZE (mode))
+	{
+	  new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
+	  HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+	  unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT) 1 << len) - 1;
+	  new_rtx = gen_rtx_AND (mode, new_rtx,
+				 gen_int_mode (mask << real_pos, mode));
+	}
+
       /* If we are have (and (rotate X C) M) and C is larger than the number
 	 of bits in M, this is an extraction.  */
 
diff --git a/gcc/testsuite/gcc.dg/pr94026.c b/gcc/testsuite/gcc.dg/pr94026.c
new file mode 100644
index 00000000000..c126567fe85
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr94026.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+foo (int c)
+{
+  int a = (c >> 8) & 7;
+
+  if (a >= 2) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0)
+   to (compare (and (x 1536)) 0). With that transformation, the following
+   aarch64 code sequence:
+        asr     w0, w0, 8
+        tst     w0, 6
+        cset    w0, ne
+        ret
+
+   should be simplified into:
+        tst     x0, 1536
+        cset    w0, ne
+        ret
+*/
+
+/* { dg-final { scan-assembler-not "asr\tw\[0-9\]+, w\[0-9\]+, 8" { target aarch64*-*-* } } } */
-- 
2.19.1