Page MenuHomePhabricator

[experimental] Add support for live-in semantics of values in deopt bundles
ClosedPublic

Authored by reames on Aug 29 2016, 11:19 AM.

Details

Summary

This is a first step towards supporting deopt value lowering and reporting entirely with the register allocator. I hope to build on this in the near future to support live-on-return semantics, but I have a use case which allows me to test and investigate code quality with just the live-in semantics so I've chosen to start there. For those curious, my use cases is our implementation of the "__llvm_deoptimize" function we bind to @llvm.deoptimize. I'm choosing not to hard code that fact in the patch and instead make it configurable via function attributes.

The basic approach here is modelled on what is done for the "Live In" values on stackmaps and patchpoints. (A secondary goal here is to remove one of the last barriers to merging the pseudo instructions.) We start by adding the operands directly to the STATEPOINT SDNode. Once we've lowered to MI, we extend the remat logic used by the register allocator to fold virtual register uses into StackMap::Indirect entries as needed. This does rely on the fact that the register allocator rematerializes. If it didn't along some code path, we could end up with more vregs than physical registers and fail to allocate.

Today, we *only* fold in the register allocator. This can create some weird effects when combined with arguments passed on the stack because we don't fold them appropriately. I have an idea how to fix that, but it needs this patch in place to work on that effectively. (There's some weird interaction with the scheduler as well, more investigation needed.)

My near term plan is to land this patch off-by-default, experiment in my local tree to identify any correctness issues and then start fixing codegen problems one by one as I find them. Once I have the live-in lowering fully working (both correctness and code quality), I'm hoping to move on to the live-on-return semantics. Note: I don't have any *known* miscompiles with this patch enabled, but I'm pretty sure I'll find at least a couple. Thus, the "experimental" tag and the fact it's off by default.

p.s. I'm not particularly attached to any of the attribute names or any of the other how aspects of how we decide to enable. If anyone has a better idea here, please make suggestions.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 69589.Aug 29 2016, 11:19 AM
reames retitled this revision from to [experimental] Add support for live-in semantics of values in deopt bundles.
reames updated this object.
reames added a subscriber: llvm-commits.
sanjoy edited edge metadata.Aug 29 2016, 7:09 PM

I don't have any deep problems with the patch, only some minor stylistic issues.

PS: This patch is one in a thousand. :)

include/llvm/IR/Statepoint.h
37 ↗(On Diff #69589)

Doxygen comments here.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
38 ↗(On Diff #69589)

I've never seen a cl::desc phrased as a question, perhaps phrase it as "If true, lower deopt bundles as live-in"?

edit: After reading the code, I'm not sure if this is needed. IIUC clients can already opt out of the new behavior by not specifying "deopt-lowering", so I'm not sure we need another knob.

455 ↗(On Diff #69589)

Is this new assert related to this change? (It's fine to land with this change or separately, I'm just curious)

Also the convention for assertion failure messages in the rest of LLVM seems to be "Pointer without base(?|!)", so we should not deviate from that unless we need to.

463 ↗(On Diff #69589)

s/conservative/conservatively/

465 ↗(On Diff #69589)

Please add a small blurb on the difference between live-out and live-through. As we discussed in person, this can be subtle.

473 ↗(On Diff #69589)

Not sure if hasBitSet is making things clearer. Does UseLiveInDeopt && (SI.StatepointFlags & StatepointFlags::DeoptLiveIn) compile?

476 ↗(On Diff #69589)

Can this be

assert(is_contained(SI.Bases, V) implies is_contained(SI.Ptrs, V));
return is_contained(SI.Ptrs, V);

?

514 ↗(On Diff #69589)

I'd add a /*LiveInOnly=*/ before the false, here and below.

lib/CodeGen/TargetInstrInfo.cpp
454 ↗(On Diff #69589)

s/arguements/arguments/
s/args/arguments/

455 ↗(On Diff #69589)

s/opers/Opers/

or

StartIdx = StatepointOpers(&MI).getVarIdx().

(I've fixed the existing violations).

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1277 ↗(On Diff #69589)

Not sure if the static is adding much here. I'd just stick with const char *DeoptLowering = "deopt-lowering". Also LLVM coding style associates the * with the variable name.

1340 ↗(On Diff #69589)

Usually assertion failure messages are formatted as "Unsupported value!".

Updated patch to follow shortly.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
455 ↗(On Diff #69589)

It's new. It should already hold, but I noticed it while working on this.

473 ↗(On Diff #69589)

Good catch. This was from I originally was trying to support live-out in the same patch and needed two bits to encode the options. Simplified. (In retrospect, I'd also picked a terrible name for the original purpose.)

476 ↗(On Diff #69589)

No it can't. You can have a single use of the derived pointer after the call. This would produce a statepoint with a single gc.relocate with the pair (base, derived), but not (base, base). This is correct code generation for this case.

reames updated this revision to Diff 69732.Aug 30 2016, 11:29 AM
reames edited edge metadata.

Address Sanjoy's comments

sanjoy accepted this revision.Aug 30 2016, 4:57 PM
sanjoy edited edge metadata.

lgtm with a couple of minor typos pointed out inline

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
461 ↗(On Diff #69732)

s/transative/transitive/

462 ↗(On Diff #69732)

s/particularly/particular/

465 ↗(On Diff #69732)

s/code generate/code generator/

This revision is now accepted and ready to land.Aug 30 2016, 4:57 PM
This revision was automatically updated to reflect the committed changes.