Page MenuHomePhabricator

Support invoking the patchpoint intrinsic

Authored by kmod on Oct 1 2014, 7:13 PM.



This is a barebones patch that enables the invoking of llvm.experimental.patchpoint intrinsics; it's missing some things (tests, should refactor the existing code instead of duplicating more of it), but I wanted to send out the current version for early feedback.

I tried to just take the invoke-handling sections from the normal Invoke path and add them to patchpoints, and it seems to work (LLVM and Pyston tests pass), but it's hard for me to verify that I'm doing this correctly and that it's not missing anything.

There might also need to be some discussion about whether it makes sense to invoke patchpoints in the first place -- I think it definitely makes sense to attach stackmap information to an invoke, but patching and statically emitting EH information might not be coherent concepts, though on my system it ends up working out fine.

Diff Detail

Event Timeline

kmod updated this revision to Diff 14308.Oct 1 2014, 7:13 PM
kmod retitled this revision from to Support invoking the patchpoint intrinsic.
kmod updated this object.
kmod edited the test plan for this revision. (Show Details)
kmod added reviewers: atrick, ributzka.
kmod added a subscriber: Unknown Object (MLST).
atrick edited edge metadata.Oct 2 2014, 12:08 AM

I think patching an exception throwing call is valid. You can certainly have an inline cache to an exception throwing method.

I also think it's valid to use the platform's exception handling support, although probably not performant if your app throws exceptions. As Filip mentioned, the best approach is to profile the throws, deoptimize when they aren't expected and emit branches when they are.

Anyway, great work. Please add tests and cleanup the code as you mentioned.

reames added a subscriber: reames.Oct 3 2014, 3:43 PM

I think patchpoints should be invokable. I like the general direction, but have not reviewed the details of the patchpoint lowering changes.


Extremely minor, but a code pattern of:
if ( is intrinsic)

switch(intrinsic_id) {
case patchpoint, patchpoint_void:
case do_nothing: break;
default: assert...

Would be more idiomatic and easier to extend.

kmod abandoned this revision.Oct 6 2014, 5:26 PM

I switched from the web interface to arc, and did not succeed in getting it to update the existing revision; closing this one in favor of the new one