[IR] Don't mark experimental.guard as willreturn

[nikic]

Control flow does not necessary continue past guard intrinsics, so don't mark them as willreturn.

This fixes the miscompile in the sdiv-guard.ll test.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Nikita Popov (nikic)

Changes

Control flow does not necessary continue past guard intrinsics, so don't mark them as willreturn.

This fixes the miscompile in the sdiv-guard.ll test.

---

Full diff: https://github.com/llvm/llvm-project/pull/69433.diff

4 Files Affected:

• (modified) llvm/include/llvm/IR/Intrinsics.td (+1-1)
• (modified) llvm/lib/Transforms/Utils/Local.cpp (+11-8)
• (modified) llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll (-1)
• (modified) llvm/test/Transforms/InstCombine/sdiv-guard.ll (+3-2)

diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index ab15b1f1e0ee888..b22da112f578fd6 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1697,7 +1697,7 @@ def int_experimental_deoptimize : Intrinsic<[llvm_any_ty], [llvm_vararg_ty],
                                             [Throws]>;
 
 // Support for speculative runtime guards
-def int_experimental_guard : DefaultAttrsIntrinsic<[], [llvm_i1_ty, llvm_vararg_ty],
+def int_experimental_guard : Intrinsic<[], [llvm_i1_ty, llvm_vararg_ty],
                                        [Throws]>;
 
 // Supports widenable conditions for guards represented as explicit branches.
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index ddb47e693a643d8..e0467a319caa302 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -443,9 +443,16 @@ bool llvm::wouldInstructionBeTriviallyDead(const Instruction *I,
     if (!II)
       return false;
 
+    switch (II->getIntrinsicID()) {
+    case Intrinsic::experimental_guard: {
+      // Guards on true are operationally no-ops.  In the future we can
+      // consider more sophisticated tradeoffs for guards considering potential
+      // for check widening, but for now we keep things simple.
+      auto *Cond = dyn_cast<ConstantInt>(II->getArgOperand(0));
+      return Cond && Cond->isOne();
+    }
     // TODO: These intrinsics are not safe to remove, because this may remove
     // a well-defined trap.
-    switch (II->getIntrinsicID()) {
     case Intrinsic::wasm_trunc_signed:
     case Intrinsic::wasm_trunc_unsigned:
     case Intrinsic::ptrauth_auth:
@@ -484,13 +491,9 @@ bool llvm::wouldInstructionBeTriviallyDead(const Instruction *I,
       return false;
     }
 
-    // Assumptions are dead if their condition is trivially true.  Guards on
-    // true are operationally no-ops.  In the future we can consider more
-    // sophisticated tradeoffs for guards considering potential for check
-    // widening, but for now we keep things simple.
-    if ((II->getIntrinsicID() == Intrinsic::assume &&
-         isAssumeWithEmptyBundle(cast<AssumeInst>(*II))) ||
-        II->getIntrinsicID() == Intrinsic::experimental_guard) {
+    // Assumptions are dead if their condition is trivially true.
+    if (II->getIntrinsicID() == Intrinsic::assume &&
+        isAssumeWithEmptyBundle(cast<AssumeInst>(*II))) {
       if (ConstantInt *Cond = dyn_cast<ConstantInt>(II->getArgOperand(0)))
         return !Cond->isZero();
 
diff --git a/llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll b/llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll
index 562b1a4a7182537..2aa95216a665694 100644
--- a/llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll
+++ b/llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll
@@ -185,7 +185,6 @@ declare void @llvm.experimental.guard(i1, ...)
 ; CHECK: attributes #[[ATTR0]] = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) }
 ; CHECK: attributes #[[ATTR1:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }
 ; CHECK: attributes #[[ATTR2]] = { nounwind }
-; CHECK: attributes #[[ATTR3:[0-9]+]] = { nocallback nofree nosync willreturn }
 ;.
 ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 ; CGSCC: {{.*}}
diff --git a/llvm/test/Transforms/InstCombine/sdiv-guard.ll b/llvm/test/Transforms/InstCombine/sdiv-guard.ll
index ba9670924108b19..cff2f6aefda06a3 100644
--- a/llvm/test/Transforms/InstCombine/sdiv-guard.ll
+++ b/llvm/test/Transforms/InstCombine/sdiv-guard.ll
@@ -6,8 +6,9 @@ declare void @llvm.experimental.guard(i1, ...)
 ; Regression test. If %flag is false then %s == 0 and guard should be triggered.
 define i32 @a(i1 %flag, i32 %X) nounwind readnone {
 ; CHECK-LABEL: @a(
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32 [[X:%.*]], 0
-; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[CMP]]) #[[ATTR2:[0-9]+]] [ "deopt"() ]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ne i32 [[X:%.*]], 0
+; CHECK-NEXT:    [[CMP:%.*]] = select i1 [[FLAG:%.*]], i1 [[CMP1]], i1 false
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[CMP]]) #[[ATTR1:[0-9]+]] [ "deopt"() ]
 ; CHECK-NEXT:    [[R:%.*]] = sdiv i32 100, [[X]]
 ; CHECK-NEXT:    ret i32 [[R]]
 ;

[annamthomas]

thank you for fixing this! After discussing with our team, we'll put up an RFC in a month or so (it would give some time for the "widenable.condition" work to bake in) to remove the experimental.guard support upstream. This should avoid maintainer burden and also save some compile time since we will no longer have to work with guards in passes.

[nikic]

thank you for fixing this! After discussing with our team, we'll put up an RFC in a month or so (it would give some time for the "widenable.condition" work to bake in) to remove the experimental.guard support upstream. This should avoid maintainer burden and also save some compile time since we will no longer have to work with guards in passes.

Very happy to hear this!