Hi, > -----Original Message----- > From: Segher Boessenkool [mailto:seg...@kernel.crashing.org] > Sent: Tuesday, May 26, 2020 11:32 PM > 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...
>
> 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.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-27 Felix Yang <felix.y...@huawei.com>
+
+ PR rtl-optimization/94026
+ * gcc.dg/pr94026.c: New test.
pr94026-v5.diff
Description: pr94026-v5.diff
