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

["Yangfei (Felix)" <felix.yang@huawei.com>]

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