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

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

Hi,

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Saturday, March 14, 2020 12:07 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
> 
> On Fri, Mar 13, 2020 at 03:21:18AM +0000, Yangfei (Felix) wrote:
> > > On Wed, Mar 04, 2020 at 08:39:36AM +0000, Yangfei (Felix) wrote:
> > > >   This is a simple fix for PR94026.
> > > >   With this fix, combine will try make an extraction if we are in
> > > > a equality
> > > comparison and this is an AND
> > > >   with a constant which is power of two minus one.  Shift here
> > > > should be an
> > > constant.  For example, combine
> > > >   will transform (compare (and (lshiftrt x 8) 6) 0) to (compare
> > > > (zero_extract
> > > (x 2 9)) 0).
> > >
> > > Why is that a good thing?
> >
> > The reported test case is reduced from spec2017 541.leela_r.  I have pasted
> original code snippet on the bugzilla.
> > We found other compilers like aocc/llvm can catch this pattern and simplify it.
> 
> That wasn't my question, let me rephrase: why would writing it as zero_extract
> (instead of as a more canonical form) be wanted?

Sorry for not getting your point here. 

> The aarch backend only has zero_extract formulations for most of the bitfield
> instructions.  If you fix that problem, all of this should go away?  Like, the
> testcase in the PR starts with
> 
> Trying 7 -> 8:
>     7: r99:SI=r103:SI>>r104:SI#0
>       REG_DEAD r104:SI
>       REG_DEAD r103:SI
>     8: r100:SI=r99:SI&0x6
>       REG_DEAD r99:SI
> Failed to match this instruction:
> (set (reg:SI 100)
>     (and:SI (ashiftrt:SI (reg:SI 103)
>             (subreg:QI (reg:SI 104) 0))
>         (const_int 6 [0x6])))
> 
> and that should match already (that's an ubfm (ubfx))?

For aarch64, if we use "ubfm/ubfx" for the reduced test case, then we still need to do a compare with zero.  Then we won't get the benefit.  
For aarch64, we need to emit a "tst" instruction here.  So we need to catch something like:  

149 (set (reg:CC_NZ 66 cc)
150     (compare:CC_NZ (and:SI (lshiftrt:SI (reg:SI 102)
151                 (const_int 8 [0x8]))
152             (const_int 6 [0x6]))
153         (const_int 0 [0])))

But this pattern is not accurate enough: we can only accept equality comparison with zero here (as indicated by the checking of equality_comparison in my original patch).  
Also, this issue is there for ports like x86.  If we go that way, then we need to handle each port affected.  
So I am inclined to handle this in an arch-independent way.  
I looked into tree phases like fwprop & fold-const before, but didn't see an appropriate point to catch this opportunity.  
Then I came to the combine phase.  

> 
> > > (There should be thorough tests on many archs, showing it helps on
> > > average, and it doesn't regress anything.  I can do that for you, but not
> right now).
> >
> > I only have aarch64 & x86_64 linux available and have tested this patch with
> spec17 on both platforms.
> > No obvious improvement & regression witnessed.  This is expected as only
> one instruction is reduced here.
> 
> What should be tested is what new combinations are done, and which are *no
> longer* done.

In theory, we won't lose but emit more zero_extract with my patch.  

> > > In general, we should have *fewer* zero_extract, not more.
> 
> Some reasons for that:
> 
> 1) All those can be expressed with simpler operations as well;
> 2) Most very similar expressions cannot be expressed as zero_extract,
> although many architectures can handle (some of) those just fine;
> 3) The optimizers do not handle zero_extract very well at all (this includes
> simplify-rtx, to start with).
> 
> sign_extract is nastier -- we really want to have a sign_extend that works on
> separate bits, not as coarse as address units as we have now -- but it currently
> isn't handled much either.

Thanks for explaining this.  I have to admit that I didn't realize this issue when I was creating my original patch.  


Felix