`llvm.experimental.stackmap` is erroneously marked `Throws`?

[vext01]

Summary

I've never used phabricator, so sorry if I do this wrong.

This is originally from:
https://bugs.llvm.org/show_bug.cgi?id=37788

---8<---
Over the last few days I have been experimenting with inserting stackmaps from
the Rust compiler.

After inserting some calls (not invokes) to llvm.experimental.stackmap into rustc's llvm
codegen, and then allowing the compiler to build itself using this change, I
was surprised to see:

Cannot invoke an intrinsic other than donothing, patchpoint, statepoint, coro_resume or coro_destroy
invoke void (i64, i32, ...%) @llvm.experimental.stackmap(i64 1, i32 0)
          to label 13

As I understand, this is LLVM's way of saying, you can't invoke (expecting a
possible exception) an intrinsic which cannot raise an exception.

Having spoke to one of the Rust developers about this, and after trying
a few things, it seems that LLVM's inlining pass is itself allowing calls to
the stackmap intrinsic to be translated to invokes. Then the verifier rejects
the resulting code.

We suspect that the fix is as follows:

diff --git a/include/llvm/IR/Intrinsics.td b/include/llvm/IR/Intrinsics.td
index a2a1f26292c..9bbe22645ac 100644
--- a/include/llvm/IR/Intrinsics.td
+++ b/include/llvm/IR/Intrinsics.td
@@ -720,8 +720,7 @@ def int_invariant_group_barrier : Intrinsic<[llvm_anyptr_ty],
 //===------------------------ Stackmap Intrinsics -------------------------===//
 //
 def int_experimental_stackmap : Intrinsic<[],
-                                  [llvm_i64_ty, llvm_i32_ty, llvm_vararg_ty],
-                                  [Throws]>;
+                                  [llvm_i64_ty, llvm_i32_ty, llvm_vararg_ty], []>;
 def int_experimental_patchpoint_void : Intrinsic<[],
                                                  [llvm_i64_ty, llvm_i32_ty,
                                                   llvm_ptr_ty, llvm_i32_ty,

In other words, remove Throws from the signature of int_experimental_stackmap.

With this patch applied, I managed to run a stage 2 rustc build without the
verifier getting upset.
--->8---

Since then I've also checked if tests work. They all pass.

[asb]

Adding Philip Reames as a reviewer, who kindly offered on Twitter to take a look at this.

[reames]

Unfortunately, I don't this can land. Let me explain why, it took me a long time to talk myself into this. Maybe you can convince me I'm wrong. :)

Essentially the core question comes down to what the use case a runtime might have for a stackmap is. Essentially, the concern comes down to what the use case for destructive patching is. The most general use case I can think of is to allow code running within a compiled frame to exit back to a lower tier of execution (deoptimize). If we want to support this case, we have to account for the execution of arbitrary code after the destructive patch and redirect. In particular, that arbitrary code might end up throwing an exception through the abstract frame represented by the current compilation. As such, the stackmap site does need to be marked as potentially throwing.

If I follow this line of reasoning to it's conclusion, we end up with a situation where the inliner should handle the inline of such calls specially, not by make them into invokes, but leave them as potentially throwing calls. (Since conceptually we might be looking at a specialization of the calling frame, chose to deoptimize the callee abstract frame and end up returning to an alternate caller specialization which handles the exception and rethrows to *it''s* caller.)

So, still a bug, just not the one we thought we had. Worth noting is that we have exactly that handling for guards and explicit calls to deoptimize, just not stackmap and patchpoint.

Sorry this took so long to review, I hit a conceptual question here I had to think through.

Worth noting is that if all of your stackmaps truly are nounwind, you can use the attribute on the call sites directly. This is a bit more verbose than changing the default, but should work out of the box.

[vext01]

Hi @reames ,

Thanks for looking into this. I *think* I understand.

Another bug which stopped me from using stackmaps is:
https://bugs.llvm.org/show_bug.cgi?id=38277

Do you know if anyone is actually using the stackmap functionality? As far as I know JavaScriptCore used to use it, but have since moved on. It worked for them because they had only one compilation unit, but as far as I can see, stackmaps do not generalise to >1 CU, making them very limited.

I wonder if stackmap support should be removed, or at least generalised for >1CU?

[reames]

In D48602#1198563, @vext01 wrote:

Do you know if anyone is actually using the stackmap functionality? As far as I know JavaScriptCore used to use it, but have since moved on.

Not that I know of. We (Azul) are using the statepoint infrastructure which shares many of the same ideas, but we are not using either STACKMAP or PATCHPOINT. I suspect we could generalize if needed, patches and discussion welcome!

As a practical matter, I tend to be horrible at replying to email. Would you want to discuss future plans here on a call to make sure we're on the same page?

[vext01]

Hi @reames,

I'd be happy to speak on a call. Perhaps drop me an email to arrange a date and time? My address is edd (at) theunixzoo.co.uk.

I'm based in the UK, so, time zones will matter :)

include/llvm/IR/Intrinsics.td

 //===------------------------ Stackmap Intrinsics -------------------------===//
 //
 def int_experimental_stackmap : Intrinsic<[],
-                                  [llvm_i64_ty, llvm_i32_ty, llvm_vararg_ty],
-                                  [Throws]>;
+                                  [llvm_i64_ty, llvm_i32_ty, llvm_vararg_ty], []>;
 def int_experimental_patchpoint_void : Intrinsic<[],
                                                  [llvm_i64_ty, llvm_i32_ty,
                                                   llvm_ptr_ty, llvm_i32_ty,