[PATCH] D58049: [InstCombine] Fix matchRotate bug when one operand is a ConstantExpr shift

[Andrew Scheidecker via Phabricator via llvm-commits]

AndrewScheidecker created this revision.
AndrewScheidecker added a reviewer: spatel.
Herald added subscribers: kristof.beyls, javed.absar.
Herald added a project: LLVM.

This bug seems to be harmless in release builds, but will cause an error in UBSAN builds or an assertion failure in debug builds.

When it gets to this opcode comparison, it assumes both of the operands are BinaryOperators, but the prior m_LogicalShift will also match a ConstantExpr. The cast<BinaryOperator> will assert in a debug build, or reading an invalid value for BinaryOp from memory with ((BinaryOperator*)constantExpr)->getOpcode() will cause an error in a UBSAN build.

The test I added will fail without this change in debug/UBSAN builds, but not in release.


Repository:
  rL LLVM

https://reviews.llvm.org/D58049

Files:
  lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  test/Transforms/InstCombine/rotate.ll


Index: test/Transforms/InstCombine/rotate.ll
===================================================================
--- test/Transforms/InstCombine/rotate.ll
+++ test/Transforms/InstCombine/rotate.ll
@@ -689,3 +689,17 @@
   ret i24 %r
 }
 
+; Test that the transform doesn't crash when there's an "or" with a ConstantExpr operand.
+
+ at external_global = external global i8
+
+define i32 @rotl_constant_expr(i32 %shamt) {
+; CHECK-LABEL: @rotl_constant_expr(
+; CHECK-NEXT:    [[SHR:%.*]] = lshr i32 ptrtoint (i8* @external_global to i32), [[SHAMT:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = or i32 [[SHR]], shl (i32 ptrtoint (i8* @external_global to i32), i32 11)
+; CHECK-NEXT:    ret i32 [[R]]
+;
+  %shr = lshr i32 ptrtoint (i8* @external_global to i32), %shamt
+  %r = or i32 %shr, shl (i32 ptrtoint (i8* @external_global to i32), i32 11)
+  ret i32 %r
+}
\ No newline at end of file
Index: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -1825,8 +1825,18 @@
       !match(Or1, m_OneUse(m_LogicalShift(m_Specific(ShVal), m_Value(ShAmt1)))))
     return nullptr;
 
-  auto ShiftOpcode0 = cast<BinaryOperator>(Or0)->getOpcode();
-  auto ShiftOpcode1 = cast<BinaryOperator>(Or1)->getOpcode();
+  auto getBinaryOperatorOrConstantExprOpcode = [](Value *V) -> unsigned {
+    if (auto TheBinaryOperator = dyn_cast<BinaryOperator>(V))
+      return unsigned(TheBinaryOperator->getOpcode());
+    else if (auto TheConstExpr = dyn_cast<ConstantExpr>(V))
+      return TheConstExpr->getOpcode();
+    else
+      llvm_unreachable(
+          "Expected Value to be either a BinaryOperator or ConstantExpr");
+  };
+
+  auto ShiftOpcode0 = getBinaryOperatorOrConstantExprOpcode(Or0);
+  auto ShiftOpcode1 = getBinaryOperatorOrConstantExprOpcode(Or1);
   if (ShiftOpcode0 == ShiftOpcode1)
     return nullptr;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58049.186232.patch
Type: text/x-patch
Size: 1975 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190211/baaf1565/attachment.bin>