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

[Segher Boessenkool <segher@kernel.crashing.org>]

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