Hi, > -----Original Message----- > From: Segher Boessenkool [mailto:seg...@kernel.crashing.org] > Sent: Tuesday, May 26, 2020 12:27 AM > To: Yangfei (Felix) <felix.y...@huawei.com> > Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaij...@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.y...@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.y...@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.y...@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.y...@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
pr94026-v4.diff
Description: pr94026-v4.diff
