[llvm] r271857 - Add safety check to InstCombiner::commonIRemTransforms

[Sanjoy Das via llvm-commits]

Author: sanjoy
Date: Sun Jun  5 16:17:04 2016
New Revision: 271857

URL: http://llvm.org/viewvc/llvm-project?rev=271857&view=rev
Log:
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.

Modified:
    llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
    llvm/trunk/test/Transforms/InstCombine/rem.ll

Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp?rev=271857&r1=271856&r2=271857&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (original)
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Sun Jun  5 16:17:04 2016
@@ -1377,8 +1377,17 @@ Instruction *InstCombiner::commonIRemTra
         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.

Modified: llvm/trunk/test/Transforms/InstCombine/rem.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/rem.ll?rev=271857&r1=271856&r2=271857&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/InstCombine/rem.ll (original)
+++ llvm/trunk/test/Transforms/InstCombine/rem.ll Sun Jun  5 16:17:04 2016
@@ -254,3 +254,119 @@ if.end:
   %rem = srem i32 %lhs, 5
   ret i32 %rem
 }
+
+ at a = common global [5 x i16] zeroinitializer, align 2
+ at 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
+}