Page MenuHomePhabricator

Enable invoking the patchpoint intrinsic

Authored by kmod on Oct 6 2014, 5:20 PM.



Split into three commits:

Reduce code duplication between patchpoint and non-patchpoint lowering. NFC.
Enable invoking the patchpoint intrinsic
Fix an over-negation bug.

Diff Detail


Event Timeline

kmod updated this revision to Diff 14479.Oct 6 2014, 5:20 PM
kmod retitled this revision from to Enable invoking the patchpoint intrinsic.
kmod updated this object.
kmod edited the test plan for this revision. (Show Details)
kmod added reviewers: ributzka, atrick.
kmod added subscribers: reames, Unknown Object (MLST).
kmod updated this revision to Diff 14540.Oct 7 2014, 5:49 PM

Remove the refactoring changes from this CR to move to a separate one

Just noticed that phabricator doesn't let you view the per-commit diffs,
so separating this into separate phabricator revisions

kmod updated this revision to Diff 14554.Oct 8 2014, 3:21 AM

Forgot to include changes to specifying that patchpoints can now throw

Otherwise the optimizer would just convert the invokes back to calls.

ributzka edited edge metadata.Oct 8 2014, 10:37 PM

Just a few coding style nitpicks inline.

I will take a closer look tomorrow and also run it through our test suite to see if there any regressions.

2018–2020 ↗(On Diff #14554)

We usually don't use brackets for single-statement bodies

2021–2030 ↗(On Diff #14554)

The switch statement and its cases have the same indentation.

2032–2034 ↗(On Diff #14554)


kmod updated this revision to Diff 14767.Oct 11 2014, 2:11 AM
kmod edited edge metadata.

Fix formatting

Hi Kevin,

the patch doesn’t seem to be against trunk. Could you please post an updated patch against trunk?
There seem to be also a lot of changes due to threading through ‘ImmutableCallSite’. I didn’t see in this patch why this is necessary?


kmod updated this revision to Diff 14894.Oct 14 2014, 2:54 PM

Rebase onto trunk (r219719)

kmod added a comment.Oct 14 2014, 3:00 PM

Ok, I rebased onto trunk. One thing I failed to mention is that this patch depends on another that I sent out, D5657, since I wanted to separate some refactoring changes from the actual invoke-supporting change.

I switched from a CallInst to an ImmutableCallSite since these functions can now take either a CallInst or an InvokeInst (specifically, visitPatchpoint can now be called from both visitIntrinsicCall and from visitInvoke), and my understanding was that an (Immutable)CallSite is the preferred way to support both. Happy to do it some other way, though.

Oh, that explains it. I wasn’t aware this patch depended on D5657.

ributzka accepted this revision.Oct 15 2014, 10:49 AM
ributzka edited edge metadata.

I still think not threading through ImmutableCallSite would be nicer and reduce unnecessary changes. Otherwise LGTM.

637–643 ↗(On Diff #14894)

While you are already at it, could you please clean up the sins I committed and update it to the current coding standard (camelCase for functions and CamelCase for variables)?

783 ↗(On Diff #14894)

I would add a default value for LandingPad = nullptr and not switch to ImmutableCallSite for the first argument.

This revision is now accepted and ready to land.Oct 15 2014, 10:49 AM
kmod added a comment.Oct 15 2014, 1:12 PM

Ok I will update the patch, but I'm not sure how you mean to not thread
through the ImmutableCallSite, since I thought InvokeInst is not a subclass
of CallInst?

Oh, I see. Never mind then.

kmod updated this revision to Diff 15049.Oct 16 2014, 2:28 PM
kmod edited edge metadata.

Update based on review comments

The patch didn't apply cleanly against trunk. I merged it, but now the test case is failing for me. The test case is very fragile, because it relies on temporary labels. I think you can get rid of most of these checks.

kmod updated this revision to Diff 15064.Oct 17 2014, 1:58 AM

Try to fix the target triple and make the intent of the test more clear

kmod updated this revision to Diff 15065.Oct 17 2014, 2:13 AM

Try to make the test a little bit less brittle.

I would like to check that the exception data covers
the entirety of the patchpoint region since that was something
I was worried about, so I tried to make it do so a little bit less

kmod added a comment.Oct 17 2014, 2:16 AM

Ok I rebased onto r220027, but the test is passing for me. Not that that's really a good sign for the test's robustness, but I'm curious now why it's failing. I noticed that I had used a different triple than all of the other tests used so I fixed that -- I wonder if that makes it pass for you? I'm assuming you're running this on a mac so that an incorrectly-specified triple could cause issues since this assumes ELF.

Separately, I also tried to make the test less brittle by getting rid of as many hard-coded label names as I could, but I couldn't get rid of the Ltmp0 one. I wanted to check to make sure the exception data covers the entire patchpoint, since that was one of my worries going into this, though now that I understand the code better I'd be more ok with taking that out.

ributzka closed this revision.Oct 17 2014, 10:49 AM
ributzka updated this revision to Diff 15086.

Closed by commit rL220055 (authored by @ributzka).