Page MenuHomePhabricator

Start migrating away from statepoint's inline length prefixed argument bundles
ClosedPublic

Authored by reames on May 26 2020, 4:19 PM.

Details

Summary

In the current statepoint design, we have four distinct groups of operands to the call: call args, gc transition args, deopt args, and gc args. This format prexisted the support in IR for operand bundles and was in fact one of the inspirations for the extension. However, we never went back and rearchitected statepoints to fully leverage bundles.

This change is the first in a small sequence to do so. All this does is extend the SelectionDAG lowering code to allow deopt and gc transition operands to be specified in either inline argument bundles or operand bundles.

Once this is in, I plan to do a few follow on changes:

  1. Switch RS4GC to default to the bundle representation where possible. This will involve auto-updating a bunch of tests and should greatly improve readability.

2a) Manually migrate most of the codegen tests to using bundles.
2b) Mark the inline bundles as deprecated in LangRef.

  1. Propose a new "gc" bundle type to remove the second to last embedded bundle. (Call args seem to make sense where they are, so I'm not planning to remove those.)
  2. Default RS4GC to generating the operand bundle form.
  3. Repeat 2a/2b for gc operands. 5a (test update) is the most error prone manual step unfortunately.
  4. Once all of the above has landed, and baked for a bit, change the format in LangRef and drop lowering support for the old format.

Diff Detail

Event Timeline

reames created this revision.May 26 2020, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 4:19 PM
reames updated this revision to Diff 266371.May 26 2020, 4:27 PM

Fix a couple of stylistic points noticed when looking at the web diff

anna accepted this revision.May 26 2020, 7:53 PM

LGTM. thanks for working on this! Apart from dropping all of the extra handling code in RS4GC and verifier (which itself is a really good reason for this support btw), I'm assuming this for optimizations which know about the deopt_op bundle (or gc_transition bundle) after relocations are made explicit? i.e., a general robustness patch.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2778

hmm. so we currently don't actually have any optimizations leveraging OB_gc_transition.

This revision is now accepted and ready to land.May 26 2020, 7:53 PM
reames marked an inline comment as done.May 27 2020, 9:19 AM

LGTM. thanks for working on this! Apart from dropping all of the extra handling code in RS4GC and verifier (which itself is a really good reason for this support btw), I'm assuming this for optimizations which know about the deopt_op bundle (or gc_transition bundle) after relocations are made explicit? i.e., a general robustness patch.

I can't quite parse what you meant with that last sentence.

The current plan is to use operand bundles throughout. We'll still have statepoints to wrap the actual call, but all of the special operands will stay in bundles.

I did find one minor annoyance. We apparently treat deopt bundles differently than statepoints with deopt args in the inliner and that's causing a crash with the follow up patch. I'm going to have to restructure things a bit for the next step to be NFC. (All in reference to patch not yet posted for review.)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2778

Yes, though this line is about supporting invokable statepoint points with transition bundles.