[PATCH] D48602: `llvm.experimental.stackmap` is erroneously marked `Throws`?

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

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 nothrow, 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48602