Page MenuHomePhabricator

Add "first class" lowering for deopt operand bundles

Authored by sanjoy on Mar 17 2016, 4:56 PM.



After this change, deopt operand bundles can be lowered directly by
SelectionDAG into STATEPOINT instructions (which are then lowered to a
call or sequence of nop, with an associated __llvm_stackmaps entry0.
This obviates the need to round-trip deoptimization state through
gc.statepoint via RewriteStatepointsForGC.

@majnemer: can you please take a look to check that I haven't botched
handling of "funclet" operand bundles?

Diff Detail


Event Timeline

sanjoy updated this revision to Diff 50990.Mar 17 2016, 4:56 PM
sanjoy retitled this revision from to Add "first class" lowering for deopt operand bundles.
sanjoy updated this object.
sanjoy added subscribers: llvm-commits, majnemer.
sanjoy updated this revision to Diff 50991.Mar 17 2016, 4:59 PM
  • fix minor typo in assertion message
reames edited edge metadata.Mar 18 2016, 6:00 PM

Mostly minor comments. This looks close to ready to go in.

2134 ↗(On Diff #50991)

add "yet" to comment?

2156 ↗(On Diff #50991)

We talked about this offline, but just for the record...

This code is intentionally not handling deopt operands attached to any intrinsic call. Support for @llvm.experimental.deoptimize will follow in another patch and we actively don't want to support deopt on other intrinsics at this time.

I'd add a comment to make that explicit.

6114 ↗(On Diff #50991)

It currently looks like we're actually supporting *either* deopt operands XOR funclet operands. I might remove the loop and just assert that nothing other than deopt operands are available on the deopt path.

Same in the other place this loop exist.

844 ↗(On Diff #50991)

0 as a magic value is unclear. At least pull out a named constant CallSiteArgOperandBegin.

857 ↗(On Diff #50991)

Minor code structure: I'd move the default init immediately before each conditional init to make the value flow obvious.

864 ↗(On Diff #50991)

Could this be written as:
if (SDValue ReturnVal = LowerAsStatepoint(SI))

Also, minor, I think it would be good to rename LowerAsStatepoint to either LowerAsSTATEPOINT or LowerAsStatepointPseudo. I keep getting the IR and MI uses of the name when reading the new code structure.

98 ↗(On Diff #50991)

Er, this doesn't look like 9 patchable bytes? Am I missing something?

sanjoy added inline comments.Mar 18 2016, 6:07 PM
98 ↗(On Diff #50991)

I'm only checking the prefix here, to avoid making the test case fragile around non-semantic instruction selection differences. Should I check for a specific nop encoding here instead?

sanjoy updated this revision to Diff 51096.Mar 18 2016, 6:40 PM
sanjoy marked 3 inline comments as done.
sanjoy edited edge metadata.
  • address review
sanjoy added a subscriber: rnk.Mar 18 2016, 6:40 PM
sanjoy added inline comments.
6114 ↗(On Diff #50991)

IIUC we don't need to do anything to lower the funclet operand bundle, so we're trivially lowering both. @majnemer, @rnk -- can you one of you please point out what the right behavior is here?

844 ↗(On Diff #50991)

Changed it to CS.arg_begin() - CS.op_begin()

857 ↗(On Diff #50991)

Changed to use getValueOr.

sanjoy added inline comments.Mar 21 2016, 9:54 AM
6118 ↗(On Diff #51096)

Update: I just confirmed the above with @majnemer on IRC

sanjoy updated this revision to Diff 51202.Mar 21 2016, 11:42 AM

Minor fixes:

  • CHECK: for a specific kind of nop
  • Add a couple of comments
reames accepted this revision.Mar 21 2016, 5:51 PM
reames edited edge metadata.

LGTM w/minor comments as follow on commits.

6120 ↗(On Diff #51202)

Minor: introducing a helper function which asserts if any operand bundle types not passed in are present would be a good cleanup. Please do as a separate change.

assert(!hasOperandBundleOtherThan({OB_deopt, OB_funclet});

840 ↗(On Diff #51202)

minor: I thought you said this could handle funclet?

If so, please add a test case and weaken the assert in a followon patch.

This revision is now accepted and ready to land.Mar 21 2016, 5:51 PM
This revision was automatically updated to reflect the committed changes.