[SimplifyCFG] Fold `llvm.guard(false)` to unreachable

[sanjoy]

Summary

llvm.guard(false) always bails out of the current compilation unit, so
we can prune any control flow following it.

Note: earlier I had submitted an incorrect version of this change as
D19188.

[reames]

include/llvm/Transforms/Utils/Local.h

295 ↗

(On Diff #54135)

Why can't you just use the API immediately above?

lib/Transforms/Utils/Local.cpp

1287 ↗

(On Diff #54135)

You should respect UseLLVMTrap here.

1347 ↗

(On Diff #54135)

Add a comment explain what this does rather than what it does not do.

[sanjoy]

• Address review

include/llvm/Transforms/Utils/Local.h

295 ↗

(On Diff #54135)

No specific reason; just from a readability POV I found it a little better to have a separately named utility than add a bool to an existing one. If you prefer editing changeToUnreachable instead, I can do that too (as you can tell, I won't even have to change the implementation).

In case you were asking why I cannot use the API above as is, changeToUnreachable removes I. But for guards the original instruction needs to remain, since that is how we'll get out of the compile (IOW it is more like "exit(0)" than it is like "store to null").

[reames]

include/llvm/Transforms/Utils/Local.h

295 ↗

(On Diff #54402)

I'm still missing something here. I get why you can't remove the *guard*, but why can't you remove the instruction *after* the guard? changeToUnreachable(std::next(BBI))?

[sanjoy]

You're right, there was never a need for a new utility. Fixed.

[majnemer]

lib/Transforms/Utils/Local.cpp

1331 ↗

(On Diff #54445)

An call -> A call.

1341 ↗

(On Diff #54445)

I think the usual style doesn't have spaces before the variable or around the equals:

changeToUnreachable(II->getNextNode(), /*UseLLVMTrap=*/false);

I've also seen it formatted like so:

changeToUnreachable(II->getNextNode(), /*UseLLVMTrap*/ false);

[reames]

LGTM w/remaining comments from David and my own addressed.

test/Transforms/SimplifyCFG/guards.ll

12 ↗

(On Diff #54445)

I believe this branch is redundant?

23 ↗

(On Diff #54445)

Clarify: and do not want to.

Also, add a test case for guard(true) just as a sanity check!

45 ↗

(On Diff #54445)

Naming wise: true, false are confusing. Possibly: dead_via_guard and merge? Applies to all tests.

llvm/trunk/lib/Transforms/Utils/Local.cpp

do {
 
     // Do a quick scan of the basic block, turning any obviously unreachable
     // instructions into LLVM unreachable insts.  The instruction combining pass
     // canonicalizes unreachable insts into stores to null or undef.
     for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E;++BBI){
       // Assumptions that are known to be false are equivalent to unreachable.
       // Also, if the condition is undefined, then we make the choice most
       // beneficial to the optimizer, and choose that to also be unreachable.
-      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI))
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI)) {
         if (II->getIntrinsicID() == Intrinsic::assume) {
           bool MakeUnreachable = false;
           if (isa<UndefValue>(II->getArgOperand(0)))
             MakeUnreachable = true;
           else if (ConstantInt *Cond =
                    dyn_cast<ConstantInt>(II->getArgOperand(0)))
             MakeUnreachable = Cond->isZero();
 
           if (MakeUnreachable) {
             // Don't insert a call to llvm.trap right before the unreachable.
             changeToUnreachable(&*BBI, false);
             Changed = true;
             break;
           }
         }
 
+        if (II->getIntrinsicID() == Intrinsic::experimental_guard) {
+          // A call to the guard intrinsic bails out of the current compilation
+          // unit if the predicate passed to it is false.  If the predicate is a
+          // constant false, then we know the guard will bail out of the current
+          // compile unconditionally, so all code following it is dead.
+          //
+          // Note: unlike in llvm.assume, it is not "obviously profitable" for
+          // guards to treat `undef` as `false` since a guard on `undef` can
+          // still be useful for widening.
+          if (auto *CI = dyn_cast<ConstantInt>(II->getArgOperand(0)))
+            if (CI->isZero() && !isa<UnreachableInst>(II->getNextNode())) {
+              changeToUnreachable(II->getNextNode(), /*UseLLVMTrap=*/ false);
+              Changed = true;
+              break;
+            }
+        }
+      }
+
       if (CallInst *CI = dyn_cast<CallInst>(BBI)) {
         if (CI->doesNotReturn()) {
           // If we found a call to a no-return function, insert an unreachable
           // instruction after it.  Make sure there isn't *already* one there
           // though.
           ++BBI;
           if (!isa<UnreachableInst>(BBI)) {
             // Don't insert a call to llvm.trap right before the unreachable.

llvm/trunk/test/Transforms/SimplifyCFG/guards.ll

+; RUN: opt -S -simplifycfg < %s | FileCheck %s
+
+declare void @llvm.experimental.guard(i1, ...)
+
+define i32 @f_0(i1 %c) {
+; CHECK-LABEL: @f_0(
+; CHECK-NEXT: entry:
+; CHECK-NEXT:  call void (i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+; CHECK-NEXT:  unreachable
+entry:
+  call void(i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+  ret i32 10
+}
+
+define i32 @f_1(i1 %c) {
+; Demonstrate that we (intentionally) do not simplify a guard on undef
+
+; CHECK-LABEL: @f_1(
+; CHECK: ret i32 10
+; CHECK: ret i32 20
+
+entry:
+  br i1 %c, label %true, label %false
+
+true:
+  call void(i1, ...) @llvm.experimental.guard(i1 undef) [ "deopt"() ]
+  ret i32 10
+
+false:
+  ret i32 20
+}
+
+define i32 @f_2(i1 %c, i32* %buf) {
+; CHECK-LABEL: @f_2(
+entry:
+  br i1 %c, label %guard_block, label %merge_block
+
+guard_block:
+  call void(i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+  %val = load i32, i32* %buf
+  br label %merge_block
+
+merge_block:
+  %to.return = phi i32 [ %val, %guard_block ], [ 50, %entry ]
+  ret i32 %to.return
+; CHECK: guard_block:
+; CHECK-NEXT:  call void (i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+; CHECK-NEXT:  unreachable
+
+; CHECK: merge_block:
+; CHECK-NEXT:  ret i32 50
+}
+
+define i32 @f_3(i1* %c, i32* %buf) {
+; CHECK-LABEL: @f_3(
+entry:
+  %c0 = load volatile i1, i1* %c
+  br i1 %c0, label %guard_block, label %merge_block
+
+guard_block:
+  call void(i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+  %val = load i32, i32* %buf
+  %c2 = load volatile i1, i1* %c
+  br i1 %c2, label %left, label %right
+
+merge_block:
+  %c1 = load volatile i1, i1* %c
+  br i1 %c1, label %left, label %right
+
+left:
+  %val.left = phi i32 [ %val, %guard_block ], [ 50, %merge_block ]
+  ret i32 %val.left
+
+right:
+  %val.right = phi i32 [ %val, %guard_block ], [ 100, %merge_block ]
+  ret i32 %val.right
+
+; CHECK: guard_block:
+; CHECK-NEXT:   call void (i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+; CHECK-NEXT:  unreachable
+
+; CHECK: merge_block:
+; CHECK-NEXT:  %c1 = load volatile i1, i1* %c
+; CHECK-NEXT:  [[VAL:%[^ ]]] = select i1 %c1, i32 50, i32 100
+; CHECK-NEXT:  ret i32 [[VAL]]
+}