[InstCombine] Do not fold 'and (sext (ashr X, Shift)), C' if Shift < 0

[BertalanD]

Summary

The 'and (sext (ashr X, ShiftC)), C' --> 'lshr (sext X), ShiftC'
transformation would access out of bounds bits in APInt::getLowBitsSet
if the shift count was larger than X's bit width or negative.

Fixes #56424

[fhahn]

LGTM, thanks!

llvm/test/Transforms/InstCombine/pr56424.ll

3

might be worth to switch to the new pass manager syntax (-passes=instcombine)

6

could you name the arguments instead of having them implicitly numbered?

[spatel]

llvm/test/Transforms/InstCombine/pr56424.ll

6

Nit: I find it a little easier for subsequent bug-hunting and archaeology if you put this test next to the existing tests for the buggy fold.
You can then name the test @PR56424 and/or put a link to the issue in the test comment.

[BertalanD]

Added a link to the issue, switched to the new PM syntax and named the arguments.

Thank you for the review

[BertalanD]

Whoops, totally forgot about the "Differential Revision:" trailer, sorry. Committed as ef7aed3e112b47641ca2704187d3701765a831ce.

[spatel]

In D129306#3636311, @BertalanD wrote:

Added a link to the issue, switched to the new PM syntax and named the arguments.

Thank you for the review

My suggestion was to add the test around here rather than create a new file for just this one test:
https://github.com/llvm/llvm-project/blob/ef7aed3e112b47641ca2704187d3701765a831ce/llvm/test/Transforms/InstCombine/and.ll#L1362

[BertalanD]

My suggestion was to add the test around here rather than create a new file for just this one test:
https://github.com/llvm/llvm-project/blob/ef7aed3e112b47641ca2704187d3701765a831ce/llvm/test/Transforms/InstCombine/and.ll#L1362

Argh, today's not my day then 🤦

[spatel]

In D129306#3636412, @BertalanD wrote:

My suggestion was to add the test around here rather than create a new file for just this one test:
https://github.com/llvm/llvm-project/blob/ef7aed3e112b47641ca2704187d3701765a831ce/llvm/test/Transforms/InstCombine/and.ll#L1362

Argh, today's not my day then 🤦

Not a big deal - the bug fix is the more important part and looks good. :)

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

if (match(Op0, m_OneUse(m_Or(m_Value(X), m_APInt(OrC))))) {
       APInt Together = *C & *OrC;
       Value *And = Builder.CreateAnd(X, ConstantInt::get(Ty, Together ^ *C));
       And->takeName(Op0);
       return BinaryOperator::CreateOr(And, ConstantInt::get(Ty, Together));
     }
 
     unsigned Width = Ty->getScalarSizeInBits();
     const APInt *ShiftC;
-    if (match(Op0, m_OneUse(m_SExt(m_AShr(m_Value(X), m_APInt(ShiftC)))))) {
+    if (match(Op0, m_OneUse(m_SExt(m_AShr(m_Value(X), m_APInt(ShiftC))))) &&
+        ShiftC->ult(Width)) {
       if (*C == APInt::getLowBitsSet(Width, Width - ShiftC->getZExtValue())) {
         // We are clearing high bits that were potentially set by sext+ashr:
         // and (sext (ashr X, ShiftC)), C --> lshr (sext X), ShiftC
         Value *Sext = Builder.CreateSExt(X, Ty);
         Constant *ShAmtC = ConstantInt::get(Ty, ShiftC->zext(Width));
         return BinaryOperator::CreateLShr(Sext, ShAmtC);
       }
     }

llvm/test/Transforms/InstCombine/pr56424.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s --passes=instcombine -S | FileCheck %s
+
+; This would crash if we didn't check for a negative shift.
+; https://github.com/llvm/llvm-project/issues/56424
+define i64 @PR56424(i1 %cond, i32 %arg) {
+; CHECK-LABEL: @PR56424(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret i64 0
+;
+entry:
+  br i1 %cond, label %if.then, label %if.end
+
+if.then:
+  %shr  = ashr i32 %arg, -2
+  %sext = sext i32 %shr to i64
+  br label %if.end
+
+if.end:
+  %val = phi i64 [ %sext, %if.then ], [ 0, %entry ]
+  %and = and i64 -81, %val
+  ret i64 %and
+}