[llvm] r319147 - [GVN] Prevent ScalarPRE from hoisting across instructions that don't pass control flow to successors

[Max Kazantsev via llvm-commits]

Author: mkazantsev
Date: Mon Nov 27 23:07:55 2017
New Revision: 319147

URL: http://llvm.org/viewvc/llvm-project?rev=319147&view=rev
Log:
[GVN] Prevent ScalarPRE from hoisting across instructions that don't pass control flow to successors

This is to address a problem similar to those in D37460 for Scalar PRE. We should not
PRE across an instruction that may not pass execution to its successor unless it is safe
to speculatively execute it.

Differential Revision: https://reviews.llvm.org/D38619

Modified:
    llvm/trunk/lib/Transforms/Scalar/GVN.cpp
    llvm/trunk/test/Transforms/GVN/PRE/local-pre.ll

Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=319147&r1=319146&r2=319147&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Mon Nov 27 23:07:55 2017
@@ -2255,6 +2255,20 @@ bool GVN::performScalarPRE(Instruction *
   Instruction *PREInstr = nullptr;
 
   if (NumWithout != 0) {
+    if (!isSafeToSpeculativelyExecute(CurInst)) {
+      // It is only valid to insert a new instruction if the current instruction
+      // is always executed. An instruction with implicit control flow could
+      // prevent us from doing it. If we cannot speculate the execution, then
+      // PRE should be prohibited.
+      auto It = FirstImplicitControlFlowInsts.find(CurrentBlock);
+      if (It != FirstImplicitControlFlowInsts.end()) {
+        assert(It->second->getParent() == CurrentBlock &&
+               "Implicit control flow map broken?");
+        if (OI->dominates(It->second, CurInst))
+          return false;
+      }
+    }
+
     // Don't do PRE across indirect branch.
     if (isa<IndirectBrInst>(PREPred->getTerminator()))
       return false;

Modified: llvm/trunk/test/Transforms/GVN/PRE/local-pre.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/local-pre.ll?rev=319147&r1=319146&r2=319147&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/PRE/local-pre.ll (original)
+++ llvm/trunk/test/Transforms/GVN/PRE/local-pre.ll Mon Nov 27 23:07:55 2017
@@ -1,6 +1,13 @@
 ; RUN: opt < %s -gvn -enable-pre -S | FileCheck %s
 
+declare void @may_exit() nounwind
+
+declare void @may_exit_1(i32) nounwind
+
 define i32 @main(i32 %p, i32 %q) {
+
+; CHECK-LABEL: @main(
+
 block1:
     %cmp = icmp eq i32 %p, %q 
 	br i1 %cmp, label %block2, label %block3
@@ -20,3 +27,121 @@ block4:
 ; CHECK: %b.pre-phi = phi i32 [ %.pre, %block3 ], [ %a, %block2 ]
 ; CHECK-NEXT: ret i32 %b.pre-phi
 }
+
+; Don't PRE across implicit control flow.
+define i32 @test2(i32 %p, i32 %q) {
+
+; CHECK-LABEL: @test2
+; CHECK: block1:
+
+block1:
+  %cmp = icmp eq i32 %p, %q
+  br i1 %cmp, label %block2, label %block3
+
+block2:
+ %a = sdiv i32 %p, %q
+ br label %block4
+
+block3:
+  br label %block4
+
+; CHECK: block4:
+; CHECK-NEXT: call void @may_exit(
+; CHECK-NEXT: %b = sdiv
+; CHECK-NEXT: ret i32 %b
+
+block4:
+  call void @may_exit() nounwind
+  %b = sdiv i32 %p, %q
+  ret i32 %b
+}
+
+; Don't PRE across implicit control flow.
+define i32 @test3(i32 %p, i32 %q, i1 %r) {
+
+; CHECK-LABEL: @test3
+; CHECK: block1:
+
+block1:
+  br i1 %r, label %block2, label %block3
+
+block2:
+ %a = sdiv i32 %p, %q
+ br label %block4
+
+block3:
+  br label %block4
+
+block4:
+
+; CHECK: block4:
+; CHECK-NEXT: phi i32
+; CHECK-NEXT: call void @may_exit_1(
+; CHECK-NEXT: %b = sdiv
+; CHECK-NEXT: ret i32 %b
+
+  %phi = phi i32 [ 0, %block3 ], [ %a, %block2 ]
+  call void @may_exit_1(i32 %phi) nounwind
+  %b = sdiv i32 %p, %q
+  ret i32 %b
+
+}
+
+; It's OK to PRE an instruction that is guaranteed to be safe to execute
+; speculatively.
+; TODO: Does it make any sense in this case?
+define i32 @test4(i32 %p, i32 %q) {
+
+; CHECK-LABEL: @test4
+; CHECK: block1:
+
+block1:
+  %cmp = icmp eq i32 %p, %q
+  br i1 %cmp, label %block2, label %block3
+
+block2:
+ %a = sdiv i32 %p, 6
+ br label %block4
+
+block3:
+  br label %block4
+
+; CHECK: block4:
+; CHECK-NEXT: %b.pre-phi = phi i32
+; CHECK-NEXT: call void @may_exit(
+; CHECK-NEXT: ret i32 %b
+
+block4:
+  call void @may_exit() nounwind
+  %b = sdiv i32 %p, 6
+  ret i32 %b
+}
+
+; It is OK to PRE across implicit control flow if we don't insert new
+; instructions.
+define i32 @test5(i1 %cond, i32 %p, i32 %q) {
+
+; CHECK-LABEL: @test5
+; CHECK: block1:
+
+block1:
+  br i1 %cond, label %block2, label %block3
+
+block2:
+ %a = sdiv i32 %p, %q
+ br label %block4
+
+block3:
+  %b = sdiv i32 %p, %q
+  br label %block4
+
+; CHECK: block4:
+; CHECK-NEXT: %c.pre-phi = phi i32 [ %b, %block3 ], [ %a, %block2 ]
+; CHECK-NEXT: call void @may_exit()
+; CHECK-NEXT: ret i32 %c.pre-phi
+
+block4:
+  call void @may_exit() nounwind
+  %c = sdiv i32 %p, %q
+  ret i32 %c
+}