Gentle ping ...
> -----Original Message----- > From: Yangfei (Felix) > Sent: Wednesday, May 27, 2020 11:52 AM > To: 'Segher Boessenkool' <seg...@kernel.crashing.org> > Cc: gcc-patches@gcc.gnu.org; Zhanghaijian (A) <z.zhanghaij...@huawei.com> > Subject: RE: [PATCH PR94026] combine missed opportunity to simplify > comparisons with zero > > 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.
