Add safety check to InstCombiner::commonIRemTransforms

Since FoldOpIntoPhi speculates the binary operation to potentially each
of the predecessors of the PHI node (pulling it out of arbitrary control
dependence in the process), we can FoldOpIntoPhi only if we know the
operation doesn't have UB.

This also brings up an interesting profitability question -- the way it
is written today, commonIRemTransforms will hoist out work from
dynamically dead code into code that will execute at runtime.  Perhaps
that isn't the best canonicalization?

Fixes PR27968.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@271857 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 47430f9..5fb337d 100644 (file)
--- a/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -1377,8 +1377,17 @@ Instruction *InstCombiner::commonIRemTransforms(BinaryOperator &I) {
         if (Instruction *R = FoldOpIntoSelect(I, SI))
           return R;
       } else if (isa<PHINode>(Op0I)) {
-        if (Instruction *NV = FoldOpIntoPhi(I))
-          return NV;
+        using namespace llvm::PatternMatch;
+        const APInt *Op1Int;
+        if (match(Op1, m_APInt(Op1Int)) && !Op1Int->isMinValue() &&
+            (I.getOpcode() == Instruction::URem ||
+             !Op1Int->isMinSignedValue())) {
+          // FoldOpIntoPhi will speculate instructions to the end of the PHI's
+          // predecessor blocks, so do this only if we know the srem or urem
+          // will not fault.
+          if (Instruction *NV = FoldOpIntoPhi(I))
+            return NV;
+        }
       }
 
       // See if we can fold away this rem instruction.

diff --git a/test/Transforms/InstCombine/rem.ll b/test/Transforms/InstCombine/rem.ll
index 8011316..ae331ee 100644 (file)
--- a/test/Transforms/InstCombine/rem.ll
+++ b/test/Transforms/InstCombine/rem.ll
@@ -254,3 +254,119 @@ if.end:
   %rem = srem i32 %lhs, 5
   ret i32 %rem
 }
+
+@a = common global [5 x i16] zeroinitializer, align 2
+@b = common global i16 0, align 2
+
+define i32 @pr27968_0(i1 %c0, i32* %val) {
+; CHECK-LABEL: @pr27968_0(
+entry:
+  br i1 %c0, label %if.then, label %if.end
+
+if.then:
+  %v = load volatile i32, i32* %val
+  br label %if.end
+
+; CHECK: if.then:
+; CHECK-NOT: srem
+; CHECK:  br label %if.end
+
+if.end:
+  %lhs = phi i32 [ %v, %if.then ], [ 5, %entry ]
+  br i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b), label %rem.is.safe, label %rem.is.unsafe
+
+rem.is.safe:
+; CHECK: rem.is.safe:
+; CHECK-NEXT:  %rem = srem i32 %lhs, zext (i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b) to i32)
+; CHECK-NEXT:  ret i32 %rem
+
+  %rem = srem i32 %lhs, zext (i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b) to i32)
+  ret i32 %rem
+
+rem.is.unsafe:
+  ret i32 0
+}
+
+define i32 @pr27968_1(i1 %c0, i1 %always_false, i32* %val) {
+; CHECK-LABEL: @pr27968_1(
+entry:
+  br i1 %c0, label %if.then, label %if.end
+
+if.then:
+  %v = load volatile i32, i32* %val
+  br label %if.end
+
+; CHECK: if.then:
+; CHECK-NOT: srem
+; CHECK:  br label %if.end
+
+if.end:
+  %lhs = phi i32 [ %v, %if.then ], [ 5, %entry ]
+  br i1 %always_false, label %rem.is.safe, label %rem.is.unsafe
+
+rem.is.safe:
+  %rem = srem i32 %lhs, -2147483648
+  ret i32 %rem
+
+; CHECK: rem.is.safe:
+; CHECK-NEXT:  %rem = srem i32 %lhs, -2147483648
+; CHECK-NEXT:  ret i32 %rem
+
+rem.is.unsafe:
+  ret i32 0
+}
+
+define i32 @pr27968_2(i1 %c0, i32* %val) {
+; CHECK-LABEL: @pr27968_2(
+entry:
+  br i1 %c0, label %if.then, label %if.end
+
+if.then:
+  %v = load volatile i32, i32* %val
+  br label %if.end
+
+; CHECK: if.then:
+; CHECK-NOT: urem
+; CHECK:  br label %if.end
+
+if.end:
+  %lhs = phi i32 [ %v, %if.then ], [ 5, %entry ]
+  br i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b), label %rem.is.safe, label %rem.is.unsafe
+
+rem.is.safe:
+; CHECK: rem.is.safe:
+; CHECK-NEXT:  %rem = urem i32 %lhs, zext (i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b) to i32)
+; CHECK-NEXT:  ret i32 %rem
+
+  %rem = urem i32 %lhs, zext (i1 icmp eq (i16* getelementptr inbounds ([5 x i16], [5 x i16]* @a, i64 0, i64 4), i16* @b) to i32)
+  ret i32 %rem
+
+rem.is.unsafe:
+  ret i32 0
+}
+
+define i32 @pr27968_3(i1 %c0, i1 %always_false, i32* %val) {
+; CHECK-LABEL: @pr27968_3(
+entry:
+  br i1 %c0, label %if.then, label %if.end
+
+if.then:
+  %v = load volatile i32, i32* %val
+  br label %if.end
+
+; CHECK: if.then:
+; CHECK-NEXT:  %v = load volatile i32, i32* %val, align 4
+; CHECK-NEXT:  %phitmp = and i32 %v, 2147483647
+; CHECK-NEXT:  br label %if.end
+
+if.end:
+  %lhs = phi i32 [ %v, %if.then ], [ 5, %entry ]
+  br i1 %always_false, label %rem.is.safe, label %rem.is.unsafe
+
+rem.is.safe:
+  %rem = urem i32 %lhs, -2147483648
+  ret i32 %rem
+
+rem.is.unsafe:
+  ret i32 0
+}