[PATCH] D28276: [JumpThreading] Fix a bug affecting guards and assumes

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: sanjoy, hfinkel, apilipenko.
reames added a subscriber: llvm-commits.
Herald added a subscriber: mcrosier.

This patch fixes an issue in jump threading where we'd sometimes use circular logic to fold the condition assumed or guarded to true.  The code had an implicit assumption that a fact true at the end of a block must be true for all uses within the block.  This is blatantly false.  The net effect for guards was that we could remove a guard on a condition which wasn't actual true at runtime and miscompile.

I noticed this while working on another optimization patch.  I'm a little terrified that we never noticed this previously.


https://reviews.llvm.org/D28276

Files:
  lib/Transforms/Scalar/JumpThreading.cpp
  test/Transforms/JumpThreading/assume.ll


Index: test/Transforms/JumpThreading/assume.ll
===================================================================
--- test/Transforms/JumpThreading/assume.ll
+++ test/Transforms/JumpThreading/assume.ll
@@ -56,8 +56,50 @@
   ret i32 %retval.0
 }
 
+ at g = external global i32
+
+; Check that we do prove a fact using an assume within the block and
+; that we *don't* use circular logic to remove the assume.
+; CHECK-LABEL: @local_assume
+; CHECK: %notnull = icmp ne i32* %array, null
+; CHECK-NEXT: call void @llvm.assume(i1 %notnull)
+; CHECK-NEXT: ret void
+define void @local_assume(i32* %array) {
+  %notnull = icmp ne i32* %array, null
+  call void @llvm.assume(i1 %notnull)
+  br i1 %notnull, label %normal, label %error
+
+normal:
+  ret void
+
+error:
+  store atomic i32 0, i32* @g unordered, align 4
+  ret void
+}
+
+; Check that we do prove a fact using an assume within the block and
+; that we *don't* use circular logic to remove the guard.
+; CHECK-LABEL: @local_guard
+; CHECK: %notnull = icmp ne i32* %array, null
+; CHECK-NEXT: call void (i1, ...) @llvm.experimental.guard(i1 %notnull) [ "deopt"(i32 0) ]
+; CHECK-NEXT: ret void
+
+define void @local_guard(i32* %array) {
+  %notnull = icmp ne i32* %array, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %notnull) ["deopt" (i32 0)]
+  br i1 %notnull, label %normal, label %error
+
+normal:
+  ret void
+
+error:
+  store atomic i32 0, i32* @g unordered, align 4
+  ret void
+}
+
 ; Function Attrs: nounwind
 declare void @llvm.assume(i1) #1
+declare void @llvm.experimental.guard(i1 %cnd, ...)
 
 declare void @bar(...)
 
Index: lib/Transforms/Scalar/JumpThreading.cpp
===================================================================
--- lib/Transforms/Scalar/JumpThreading.cpp
+++ lib/Transforms/Scalar/JumpThreading.cpp
@@ -822,17 +822,24 @@
         CondBr->getSuccessor(ToRemove)->removePredecessor(BB, true);
         BranchInst::Create(CondBr->getSuccessor(ToKeep), CondBr);
         CondBr->eraseFromParent();
-        if (CondCmp->use_empty())
-          CondCmp->eraseFromParent();
-        else if (CondCmp->getParent() == BB) {
-          // If the fact we just learned is true for all uses of the
-          // condition, replace it with a constant value
+
+        // Now, see if we can eagerly update any other uses. WARNING: we used a
+        // context instruction at the end of the current basic block, so the
+        // facts LVI returned may not be true before that point.  (i.e. we may
+        // have a guard or assume part way through the block.)  Thus, we can
+        // not reason about any use which is not dominated by the context
+        // instruction.  Given we don't have dom-tree here, we're restricted to
+        // only handling cases where dominance can be trivially established
+        // from the definition of SSA.
+        if (!CondCmp->use_empty() &&
+            CondCmp->getParent() == BB) {
           auto *CI = Ret == LazyValueInfo::True ?
             ConstantInt::getTrue(CondCmp->getType()) :
             ConstantInt::getFalse(CondCmp->getType());
-          CondCmp->replaceAllUsesWith(CI);
-          CondCmp->eraseFromParent();
+          replaceNonLocalUsesWith(CondCmp, CI);
         }
+        if (CondCmp->use_empty())
+          CondCmp->eraseFromParent();
         return true;
       }
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28276.82995.patch
Type: text/x-patch
Size: 3338 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170104/058749ec/attachment.bin>