[PATCH] D63829: [InstCombine] Shift amount reassociation in bittest (PR42399)

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - but see inline comment. It might simplify the code here if we implement the other fold first.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3267-3277
+  if (!match(
+          &I,
+          m_ICmp(Pred,
+                 m_OneUse(m_c_And(
+                     m_OneUse(m_CombineAnd(
+                         m_LogicalShift(m_Value(X), m_Value(XShAmt)),
+                         m_Value(XShift))),
----------------
lebedev.ri wrote:
> spatel wrote:
> > spatel wrote:
> > > This would be easier to read for me if we did the simple icmp checks first:
> > >   if (!I.isEquality() || !match(I.getOperand(1), m_Zero()))
> > >     return nullptr;
> > > 
> > > And then do a mega-match on only I.getOperand(0).
> > > 
> > > ...although that match raises a question if the one-use checks are balanced:
> > > Do we prefer to shift left or right?
> > > https://rise4fun.com/Alive/
> > >   %t0 = lshr i32 %x, 2
> > >   %t1 = and i32 %t0, %y
> > >   %t3 = icmp ne i32 %t1, 0
> > >     =>
> > >   %v0 = shl i32 %y, 2
> > >   %v1 = and i32 %v0, %x
> > >   %t3 = icmp ne i32 %v1, 0
> > Incomplete Alive link in previous comment:
> > https://rise4fun.com/Alive/9Kp
> > ...although that match raises a question if the one-use checks are balanced:
> 
> I think they are currently.
> We will produce 3 instructions: `icmp` + `and` + `shift`.
> Therefore we need to get rid of 2 instructions (since the root `icmp` is always good to go).
> Given the nature of the pattern the `icmp` is comparing `and` with `0`, so `and` must go.
> And thus we only need to get rid of one more instruction.
> We know that both hands of an `and` are shifts.
> Since the pattern is bi-directional, we don't really care which one of these two shifts
> will be left, only one of those 2 shifts *has* to be one-use.
> And that's what is currently enforced via `m_OneUse()`.
> 
> > Do we prefer to shift left or right?
> 
> I'm not sure we can answer that here.
> If we can do this fold as per one-use checks - we should.
> Do you have anything specific in mind here?
> I'd say this should be a separate fold.
Yes, I agree that choosing the shift op is a separate fold. My guess is that 'lshr' gives us better analysis because it clears the sign bit, but that's just a hunch - would have to try it and see what breaks. :)

If we make that decision first, then we could adjust this patch to go directly to the preferred form. As it stands now, we're arbitrarily choosing the shift opcode based on operand ordering (when both sides have one use).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63829