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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added inline comments.


================
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();
----------------
spatel wrote:
> 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.)
Actually that's not a restriction since `i1` type is desirable so `shouldChangeType()` check is always true.
But ok, moved that check down for shifts only.


================
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));
----------------
spatel wrote:
> 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`).
Thanks, fixed and added test.


================
Comment at: llvm/test/Transforms/InstCombine/pr52078.ll:14
 
 define i16 @bar(i1 %a) {
 ; CHECK-LABEL: @bar(
----------------
spatel wrote:
> 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.
Ok, removed and added new tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111330