[Patchpoint] Add immarg attributes to patchpoint arguments

[Il-Capitano]

This fixes an issue where simplifycfg would merge two patchpoints, resulting in a dynamic ID being used, which triggers an assert during code generation.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Csanád Hajdú (Il-Capitano)

Changes

This fixes an issue where simplifycfg would merge two patchpoints, resulting in a dynamic ID being used, which triggers an assert during code generation.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/IR/Intrinsics.td (+6-2)
• (added) llvm/test/Transforms/SimplifyCFG/patchpoint-invalid-sink.ll (+35)

diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index c7d383a5d0c0c..332941cadb456 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1606,12 +1606,16 @@ def int_experimental_patchpoint_void : Intrinsic<[],
                                                  [llvm_i64_ty, llvm_i32_ty,
                                                   llvm_ptr_ty, llvm_i32_ty,
                                                   llvm_vararg_ty],
-                                                  [Throws]>;
+                                                  [Throws, ImmArg<ArgIndex<0>>,
+                                                   ImmArg<ArgIndex<1>>,
+                                                   ImmArg<ArgIndex<3>>]>;
 def int_experimental_patchpoint : Intrinsic<[llvm_any_ty],
                                             [llvm_i64_ty, llvm_i32_ty,
                                              llvm_ptr_ty, llvm_i32_ty,
                                              llvm_vararg_ty],
-                                            [Throws]>;
+                                            [Throws, ImmArg<ArgIndex<0>>,
+                                             ImmArg<ArgIndex<1>>,
+                                             ImmArg<ArgIndex<3>>]>;
 
 
 //===------------------------ Garbage Collection Intrinsics ---------------===//
diff --git a/llvm/test/Transforms/SimplifyCFG/patchpoint-invalid-sink.ll b/llvm/test/Transforms/SimplifyCFG/patchpoint-invalid-sink.ll
new file mode 100644
index 0000000000000..eeb5710234065
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/patchpoint-invalid-sink.ll
@@ -0,0 +1,35 @@
+; RUN: opt -passes='simplifycfg<sink-common-insts>' -S %s | FileCheck %s
+
+declare void @personalityFn()
+
+define void @test(i1 %c) personality ptr @personalityFn {
+; CHECK-LABEL: define void @test
+; CHECK-LABEL: entry:
+; CHECK-NEXT:    br i1 %c, label %taken, label %untaken
+; CHECK-LABEL: taken:
+; CHECK-NEXT:    invoke void (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.void(i64 1, i32 0, ptr null, i32 0)
+; CHECK-LABEL: untaken:
+; CHECK-NEXT:    invoke void (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.void(i64 2, i32 0, ptr null, i32 0)
+; CHECK-LABEL: end:
+; CHECK-NEXT:    ret void
+entry:
+  br i1 %c, label %taken, label %untaken
+
+taken:
+  invoke void (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.void(i64 1, i32 0, ptr null, i32 0)
+          to label %end unwind label %unwind
+
+untaken:
+  invoke void (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.void(i64 2, i32 0, ptr null, i32 0)
+          to label %end unwind label %unwind
+
+end:
+  ret void
+
+unwind:
+  %0 = landingpad { ptr, i32 }
+          cleanup
+  br label %end
+}
+
+declare void @llvm.experimental.patchpoint.void(i64 immarg, i32 immarg, ptr, i32 immarg, ...)

[Il-Capitano]

@huntergr-arm @fmayer @chapuni Pinging for review.

[Il-Capitano]

@huntergr-arm @fmayer @chapuni Ping

[fmayer]

Sorry for the delay, I was on a break, I will take a look this week.

[fmayer]

It looks fine to me, but why was I added to this? I don't have experience with patchpoint?

[Il-Capitano]

@fmayer You must have been on a blame list for some unrelated change in llvm/include/llvm/IR/Intrinsics.td. Sorry for the confusion, and thanks for taking the time anyways.

[Il-Capitano]

@sanjoy @preames @arsenm Can I ask for review of this PR, or can you add appropriate reviewers? There hasn't been much development on this part of LLVM in the past years, so it's hard to find reviewers, and I think this is a fairly trivial bugfix. Thanks!

[arsenm]

Could use coverage in test/Verifier/intrinsic-immarg.ll

[Il-Capitano]

Thanks for the review!

Could use coverage in test/Verifier/intrinsic-immarg.ll

I added a test case in the latest change.

[Il-Capitano]

Can I ask for this to be merged?