[PATCH] D42088: [x86] shrink 'and' immediate values by setting the high bits (PR35907)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added a comment.

In https://reviews.llvm.org/D42088#977421, @craig.topper wrote:

> I wonder if we should do this in PreprocessISelDAG so the killing off of the AND when the mask is all 1s doesn't seem quite so weird. Right now when it happens the Select call ends up doing selection on the input to the AND.


Let me take a closer look at how this escapes SimplifyDemandedBits. There might be a hole in our AssertZext logic. We get it right if we start with this IR:

  define i32 @test3mul(i8 zeroext %x) {
    %z = zext i8 %x to i32
    %m = mul nuw nsw i32 %z, 171
    %s = lshr i32 %m, 9
    %a = and i32 %s, 127
    ret i32 %a
  }



================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2471
+
+  SDLoc DL(And);
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
----------------
craig.topper wrote:
> One of these days we should really decide between 'DL' and 'dl'...
I thought 'dl' was a holdover from the days when variables were lower-case? I'd be happy to reformat the whole file. :)


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2473
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
+  SDValue NewAnd = CurDAG->getNode(ISD::AND, DL, VT, And0, NewMask);
+  ReplaceNode(And, NewAnd.getNode());
----------------
craig.topper wrote:
> Just to make sure I understand. If the new mask is all ones, getNode constant folds the AND away leaving the constant disconnected. But isel will notice the constant is unused and not select it? Or maybe isel doesn't see the constant at all because it wasn't there when the selection process started?
> 
> Though if we can prove the upper bits are 0 and the result is all ones why didn't simplify demanded bits during DAG combine already remove the AND. I'm just trying to understand what happened on the tests in divide-by-constant.ll
Yes, getNode has a fold for 'and X, -1' --> X. 

If we look at 'test3', we have this going in:
          t4: i32 = AssertZext t2, ValueType:ch:i8
        t41: i32 = mul t4, Constant:i32<171>
      t38: i32 = srl t41, Constant:i8<9>
    t39: i32 = and t38, Constant:i32<127>

So getNode() returns the srl (t38), that replaces the and (t39), and we select that srl now:

  Selecting: t39: i32 = and t38, Constant:i32<127>
  Creating constant: t43: i32 = Constant<-1>
  ISEL: Starting pattern match on root node: t38: i32 = srl t41, Constant:i8<9>



https://reviews.llvm.org/D42088