[llvm] d4940c0 - [InstCombine] fix miscompile from urem/udiv transform with constant expression

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2022-07-29T17:14:30-04:00
New Revision: d4940c0f3d433ca5bab66d1e6a5170059c38a10a

URL: https://github.com/llvm/llvm-project/commit/d4940c0f3d433ca5bab66d1e6a5170059c38a10a
DIFF: https://github.com/llvm/llvm-project/commit/d4940c0f3d433ca5bab66d1e6a5170059c38a10a.diff

LOG: [InstCombine] fix miscompile from urem/udiv transform with constant expression

The isa<Constant> check could misfire on an instruction with 2 constant
operands. This bug was introduced with bb789381fc11cce (D36988).

See issue #56810 for a C source example that exposed the bug.

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
    llvm/test/Transforms/InstCombine/udivrem-change-width.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 08e430694a730..abe15e2bda241 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -1008,8 +1008,7 @@ static Instruction *narrowUDivURem(BinaryOperator &I,
   }
 
   Constant *C;
-  if ((match(N, m_OneUse(m_ZExt(m_Value(X)))) && match(D, m_Constant(C))) ||
-      (match(D, m_OneUse(m_ZExt(m_Value(X)))) && match(N, m_Constant(C)))) {
+  if (match(N, m_OneUse(m_ZExt(m_Value(X)))) && match(D, m_Constant(C))) {
     // If the constant is the same in the smaller type, use the narrow version.
     Constant *TruncC = ConstantExpr::getTrunc(C, X->getType());
     if (ConstantExpr::getZExt(TruncC, Ty) != C)
@@ -1017,11 +1016,17 @@ static Instruction *narrowUDivURem(BinaryOperator &I,
 
     // udiv (zext X), C --> zext (udiv X, C')
     // urem (zext X), C --> zext (urem X, C')
+    return new ZExtInst(Builder.CreateBinOp(Opcode, X, TruncC), Ty);
+  }
+  if (match(D, m_OneUse(m_ZExt(m_Value(X)))) && match(N, m_Constant(C))) {
+    // If the constant is the same in the smaller type, use the narrow version.
+    Constant *TruncC = ConstantExpr::getTrunc(C, X->getType());
+    if (ConstantExpr::getZExt(TruncC, Ty) != C)
+      return nullptr;
+
     // udiv C, (zext X) --> zext (udiv C', X)
     // urem C, (zext X) --> zext (urem C', X)
-    Value *NarrowOp = isa<Constant>(D) ? Builder.CreateBinOp(Opcode, X, TruncC)
-                                       : Builder.CreateBinOp(Opcode, TruncC, X);
-    return new ZExtInst(NarrowOp, Ty);
+    return new ZExtInst(Builder.CreateBinOp(Opcode, TruncC, X), Ty);
   }
 
   return nullptr;

diff  --git a/llvm/test/Transforms/InstCombine/udivrem-change-width.ll b/llvm/test/Transforms/InstCombine/udivrem-change-width.ll
index 522950257b11f..88e31a29aa0f2 100644
--- a/llvm/test/Transforms/InstCombine/udivrem-change-width.ll
+++ b/llvm/test/Transforms/InstCombine/udivrem-change-width.ll
@@ -287,13 +287,13 @@ define i32 @udiv_constexpr(i8 %a) {
   ret i32 %d
 }
 
-; FIXME: This is a miscompile (minimal form of PR56810)
+; minimal form of PR56810
 
 @g1 = external global [1 x i8]
 
 define i32 @udiv_const_constexpr(i8 %a) {
 ; CHECK-LABEL: @udiv_const_constexpr(
-; CHECK-NEXT:    [[TMP1:%.*]] = udiv i8 ptrtoint ([1 x i8]* @g1 to i8), 42
+; CHECK-NEXT:    [[TMP1:%.*]] = udiv i8 42, ptrtoint ([1 x i8]* @g1 to i8)
 ; CHECK-NEXT:    [[D:%.*]] = zext i8 [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[D]]
 ;
@@ -301,13 +301,13 @@ define i32 @udiv_const_constexpr(i8 %a) {
   ret i32 %d
 }
 
-; FIXME: This is a miscompile (minimal form of PR56810)
+; minimal form of PR56810
 
 @g2 = external global [1 x i8]
 
 define i32 @urem_const_constexpr(i8 %a) {
 ; CHECK-LABEL: @urem_const_constexpr(
-; CHECK-NEXT:    [[TMP1:%.*]] = urem i8 ptrtoint ([1 x i8]* @g2 to i8), 42
+; CHECK-NEXT:    [[TMP1:%.*]] = urem i8 42, ptrtoint ([1 x i8]* @g2 to i8)
 ; CHECK-NEXT:    [[D:%.*]] = zext i8 [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[D]]
 ;