[PATCH] D111330: [InstCombine] Support arbitrary const shift amount for `lshr (sext i1 ...)`

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Can you add the instcombine tests to the file(s) where the existing transform's tests are ("lshr.ll")? It's easier to see the variations that we handle in one look that way.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1071
     if (match(Op0, m_SExt(m_Value(X))) &&
         (!Ty->isIntegerTy() || shouldChangeType(Ty, X->getType()))) {
       unsigned SrcTyBitWidth = X->getType()->getScalarSizeInBits();
----------------
We don't need the `shouldChangeType` restriction for the fold to select / zext, do we?
(I'm not sure that we need it for the fold that creates a shift either; that was conservatively added back in D33879.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1075-1076
+      if (SrcTyBitWidth == 1) {
+        auto *NewC = ConstantExpr::getLShr(ConstantInt::get(Ty, -1),
+                                           ConstantInt::get(Ty, ShAmtC));
+        return SelectInst::Create(X, NewC, ConstantInt::getNullValue(Ty));
----------------
For a wide type, this won't work the way you are expecting. Use getAllOnesValue or APInt::getLowBitsSet().

Please add a test like this to verify we don't miscompile:
  define i128 @foo(i1 %a) {
    %s = sext i1 %a to i128
    %lshr = lshr i128 %s, 42
    ret i128 %lshr
  }

This might need a data layout adjustment to trigger (or the suggestion above to remove `shouldChangeType`).


================
Comment at: llvm/test/Transforms/InstCombine/pr52078.ll:14
 
 define i16 @bar(i1 %a) {
 ; CHECK-LABEL: @bar(
----------------
This should already be covered by an existing test, so it shouldn't be necessary.
We do need other tests though: extra use on the sext, vector type, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111330