LoadPRE was not properly checking that the load it was PRE'ing post-dominated the...

Splitting critical edges at the merge point only addressed part of the issue; it is also possible for non-post-domination
to occur when the path from the load to the merge has branches in it.  Unfortunately, full anticipation analysis is
time-consuming, so for now approximate it.  This is strictly more conservative than real anticipation, so we will miss
some cases that real PRE would allow, but we also no longer insert loads into paths where they didn't exist before. :-)

This is a very slight net positive on SPEC for me (0.5% on average).  Most of the benchmarks are largely unaffected, but
when it pays off it pays off decently: 181.mcf improves by 4.5% on my machine.

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

diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp
index 41ff18702d7237429d70b62bb37cfc91c9545f86..de99c3f8b6970fd9210be4de87294cba67fe4d24 100644 (file)
--- a/lib/Transforms/Scalar/GVN.cpp
+++ b/lib/Transforms/Scalar/GVN.cpp
@@ -1532,8 +1532,14 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
       return false;
     if (Blockers.count(TmpBB))
       return false;
+    
+    // If any of these blocks has more than one successor (i.e. if the edge we
+    // just traversed was critical), then there are other paths through this 
+    // block along which the load may not be anticipated.  Hoisting the load 
+    // above this block would be adding the load to execution paths along
+    // which it was not previously executed.
     if (TmpBB->getTerminator()->getNumSuccessors() != 1)
-      allSingleSucc = false;
+      return false;
   }
 
   assert(TmpBB);

diff --git a/test/Transforms/GVN/pre-single-pred.ll b/test/Transforms/GVN/pre-single-pred.ll
index 706a16b7bdd28e0b0ad1b4f289e60219526bad0f..f1f5c71a93ab86750b414e92370d0b749b9fafca 100644 (file)
--- a/test/Transforms/GVN/pre-single-pred.ll
+++ b/test/Transforms/GVN/pre-single-pred.ll
@@ -1,4 +1,13 @@
-; RUN: opt < %s -gvn -enable-load-pre -S | not grep {tmp3 = load}
+; RUN: opt < %s -gvn -enable-load-pre -S | FileCheck %s
+; This testcase assumed we'll PRE the load into %for.cond, but we don't actually
+; verify that doing so is safe.  If there didn't _happen_ to be a load in
+; %for.end, we would actually be lengthening the execution on some paths, and
+; we were never actually checking that case.  Now we actually do perform some
+; conservative checking to make sure we don't make paths longer, but we don't
+; currently get this case, which we got lucky on previously.
+;
+; Now that that faulty assumption is corrected, test that we DON'T incorrectly
+; hoist the load.  Doing the right thing for the wrong reasons is still a bug.
 
 @p = external global i32
 define i32 @f(i32 %n) nounwind {
@@ -13,6 +22,8 @@ for.cond:             ; preds = %for.inc, %entry
 for.cond.for.end_crit_edge:            ; preds = %for.cond
        br label %for.end
 
+; CHECK: for.body:
+; CHECK-NEXT: %tmp3 = load i32* @p
 for.body:              ; preds = %for.cond
        %tmp3 = load i32* @p            ; <i32> [#uses=1]
        %dec = add i32 %tmp3, -1                ; <i32> [#uses=2]
@@ -20,6 +31,7 @@ for.body:             ; preds = %for.cond
        %cmp6 = icmp slt i32 %dec, 0            ; <i1> [#uses=1]
        br i1 %cmp6, label %for.body.for.end_crit_edge, label %for.inc
 
+; CHECK: for.body.for.end_crit_edge:
 for.body.for.end_crit_edge:            ; preds = %for.body
        br label %for.end