[PATCH] D77804: [DAG] Enable ISD::SRL SimplifyMultipleUseDemandedBits handling inside SimplifyDemandedBits (WIP)

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/test/CodeGen/ARM/uxtb.ll:112
+; CHECK-NEXT:    orr r0, r0, r1
 ; CHECK-NEXT:    bx lr
   %tmp1 = lshr i32 %p0, 7
----------------
RKSimon wrote:
> RKSimon wrote:
> > dmgreen wrote:
> > > RKSimon wrote:
> > > > RKSimon wrote:
> > > > > I'm going to take a look at this, but I'm really not familiar with the UXTB matching code, so any pointers would be appreciated.
> > > > instcombine optimises this as well:
> > > > ```
> > > > define i32 @test10(i32 %p0) {
> > > >   %tmp1 = lshr i32 %p0, 7
> > > >   %tmp2 = and i32 %tmp1, 16253176
> > > >   %tmp4 = lshr i32 %p0, 12
> > > >   %tmp5 = and i32 %tmp4, 458759
> > > >   %tmp7 = or i32 %tmp5, %tmp2
> > > >   ret i32 %tmp7
> > > > }
> > > > ```
> > > > which has the same problem:
> > > > ```
> > > > _test10:
> > > > @ %bb.0:
> > > >         mov     r1, #248
> > > >         mov     r2, #7
> > > >         orr     r1, r1, #16252928
> > > >         orr     r2, r2, #458752
> > > >         and     r1, r1, r0, lsr #7
> > > >         and     r0, r2, r0, lsr #12
> > > >         orr     r0, r0, r1
> > > >         bx      lr
> > > > ```
> > > I was taking a look. The test is super old now, so old that it had signed types when it was originally added.
> > > 
> > > I was surprised to see that `and 0x70007` is being recognised via an `and 0xff00ff` tablegen pattern - it goes into SelectionDAGISel::CheckAndMask which checks that the other mask bits are already 0.
> > > 
> > > I think that is what this is trying to test - that a smaller and mask still matches the UXTB16. Is it possible to change it to something that still captures that, without relying on the multi-use fold of the %tmp2 not happening?
> > > 
> > > Maybe something like this?
> > > ```
> > >   %p = and i32 %p0, 3
> > >   %a = shl i32 65537, %p
> > >   %b = lshr i32 %a, 1
> > >   %tmp7 = and i32 %b, 458759
> > > ```
> > Thanks for the hint - I'll give it a try
> Thanks @dmgreen - those still match fine. Should I pre-commit these new tests and possibly alter the existing test10 variants with the -instcombine optimized IR to show they already fail to match?
That sounds good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77804/new/

https://reviews.llvm.org/D77804