This is an archive of the discontinued LLVM Phabricator instance.

Teach the inliner to track deoptimization state
ClosedPublic

Authored by sanjoy on Nov 10 2015, 1:16 PM.

Details

Summary

This change teaches LLVM's inliner to track and suitably adjust
deoptimization state (tracked via deoptimization operand bundles) as it
inlines through call sites. The operation is described in more detail
in the LangRef changes.

Note:
The changes to CALLSITE_DELEGATE_GETTER were needed to allow
unification of the CallInst * and InvokeInst * types into an
Instruction * type in cloneWithOperandBundles.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 39848.Nov 10 2015, 1:16 PM
sanjoy retitled this revision from to Teach the inliner to track deoptimization state.
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.
sanjoy updated this object.Nov 10 2015, 1:20 PM
reames added inline comments.Nov 10 2015, 2:50 PM
docs/LangRef.rst
1541 ↗(On Diff #39848)

I'd add something here about it being the frontend responsibility to ensure that the bundle format is sufficient so that individual abstract frames can be reconstructed from the concatenated form if required.

include/llvm/IR/CallSite.h
215 ↗(On Diff #39848)

This really looks like a white space change?

If you're going to introduce control flow, please use the "do { X } while(false)" idiom to make it less prone to miscompies.

include/llvm/IR/Instructions.h
1463 ↗(On Diff #39848)

This comment is unclear. Is the argument list a white list? Is it an entirely new list of operand bundles? If so, where'd it come from?

lib/IR/Instructions.cpp
301 ↗(On Diff #39848)

Don't we have an args_begin(), args_end?

Or better, an args() which returns an ArrayRef?

585 ↗(On Diff #39848)

Same as above?

lib/Transforms/Utils/InlineFunction.cpp
211 ↗(On Diff #39848)

This really seems like we need a getOperandBundles that returns an ArrayRef?

1039 ↗(On Diff #39848)

I'd just inline this into the if and swap the comparison directions.

p.s. Don't you need to worry about a bundle type in the callee which is not deopt?

1151 ↗(On Diff #39848)

I'm not sure about the order of the code here, but the fact we're replacing the same instruction twice (once for call to invoke, once for deopt bundles) seems likely to be error prone, particularly with ValueHandles being involved. Is there a way to avoid the complexity here? Don't have an obvious one myself, just asking.

1175 ↗(On Diff #39848)

This is already enforced by the verifier, please remove.

1185 ↗(On Diff #39848)

TagName? Why not tagID? And why not hard code OB_deopt here since it's implied by the surrounding code?

reames requested changes to this revision.Nov 10 2015, 2:50 PM
reames edited edge metadata.
This revision now requires changes to proceed.Nov 10 2015, 2:50 PM

Replies inline.

Summarizing my plan to address the more complex review commments, I've proposed three post-commit changes inline:

  • Change OperandBundleDef such that it can (alternative to containing an std::string tag name) contain an integral tag ID
  • Change CALLSITE_DELEGATE_SETTER and CALLSITE_DELEGATE_GETTER to use the safer do { ... } while (false) idiom
  • Introduce an iterator for getOperandBundleAt(I) and use that as much as possible
include/llvm/IR/CallSite.h
215 ↗(On Diff #39848)

I'm just copying the pattern from CALLSITE_DELEGATE_SETTER. I agree that the do { ... } while (false) form is better -- is it okay if I fix CALLSITE_DELEGATE_SETTER and CALLSITE_DELEGATE_GETTER together in a separate checkin?

include/llvm/IR/Instructions.h
1463 ↗(On Diff #39848)

It is an entirely new list of operand bundles. Will update the doc.

lib/IR/Instructions.cpp
301 ↗(On Diff #39848)

They're there on CallSite, not on CallInst. Does changing this to

std::vector<...> Args = CallSite(this).args()

sound reasonable?

585 ↗(On Diff #39848)

Same reply as above.

lib/Transforms/Utils/InlineFunction.cpp
211 ↗(On Diff #39848)

That's difficult -- the object returned by getOperandBundleAt is a flyweight object (like StringRef etc.) that does not have a direct backing store (i.e. there is no internal array of OperandBundleUse objects) that I can create an ArrayRef<OperandBundleUse> with. I can definitely write a stateful iterator that iterates through all the operand bundles for a CallSite, but I've been waiting to do that sort of thing after bring-up.

1039 ↗(On Diff #39848)

I'd just inline this into the if and swap the comparison directions.

SGTM.

p.s. Don't you need to worry about a bundle type in the callee which is not deopt?

The current semantics is that the caller's deopt state gets prepended to all of the "deopt" operand bundles in the callee. Call sites in the callee that have arbitrary deopt operand bundles are left intact. The test cases demonstrate this.

1151 ↗(On Diff #39848)

I don't think there should be problems with ValueHandle s, but I do think this is somewhat inefficient. However, I don't think how I can fix that without changing CloneFunction to be aware of whether it is inlining into a call site that's a call instruction, or an invoke; and changing that is outside the scope of this patch.

1185 ↗(On Diff #39848)

Right now the OperandBundleDef structure only carries an std::string that gets re-interned into LLVMContextImpl when the host instruction is constructed -- it has no way of holding a direct integer ID. This needs to be fixed (for performance, if nothing else), but doing that in this change will make it more complex. How about I change ChildOB.getTagName() to "deopt" for now, and in a later change add the functionality to create an OperandBundleDef holding a direct integral ID?

sanjoy updated this revision to Diff 39876.Nov 10 2015, 5:45 PM
sanjoy edited edge metadata.
sanjoy marked 2 inline comments as done.
sanjoy updated this object.
  • address review
lib/Transforms/Utils/InlineFunction.cpp
1039 ↗(On Diff #39848)

I'd just inline this into the if and swap the comparison directions.

I decided to keep this as-is (with a fix to the comment). I like how it reads currently -- "if CS has operand bundles attached, it better be of this form". Let me know if you feel strongly otherwise.

gentle ping!

Responses to minor comments inline.

  • Change OperandBundleDef such that it can (alternative to containing an std::string tag name) contain an integral tag ID

Seems useful. SGTM

  • Change CALLSITE_DELEGATE_SETTER and CALLSITE_DELEGATE_GETTER to use the safer do { ... } while (false) idiom

SGTM

  • Introduce an iterator for getOperandBundleAt(I) and use that as much as possible

Not sure this is worth it.

lib/IR/Instructions.cpp
301 ↗(On Diff #39876)

Yes.

lib/Transforms/Utils/InlineFunction.cpp
1153 ↗(On Diff #39876)

Makes sense.

1187 ↗(On Diff #39876)

SGTM

reames added inline comments.Nov 17 2015, 3:17 PM
include/llvm/IR/Instructions.h
1469 ↗(On Diff #39876)

Offline, we decided a Create(InvokeInst, OperandBundles, InsertPt) would be a cleaner interface here.

lib/Transforms/Utils/InlineFunction.cpp
211–217 ↗(On Diff #39876)

Offline we realized the confusion here. This loop is converting uses to defs, and is not simply a copy.

sanjoy updated this revision to Diff 40460.Nov 17 2015, 5:02 PM
sanjoy edited edge metadata.
  • address review
sanjoy updated this revision to Diff 40461.Nov 17 2015, 5:12 PM
  • use a literal "deopt" string as the tag
reames accepted this revision.Nov 17 2015, 5:49 PM
reames edited edge metadata.

LGTM w/comments addressed. I don't think this needs another round of review.

include/llvm/IR/Instructions.h
1463 ↗(On Diff #40461)

The difference in insertion logic here is confusing and different from every other create function. Please stick to convention.

lib/Transforms/Utils/CloneFunction.cpp
386 ↗(On Diff #40461)

CodeInfo might be null, please guard here and other as well.

lib/Transforms/Utils/InlineFunction.cpp
1171 ↗(On Diff #40461)

minor: reserve final size

1175 ↗(On Diff #40461)

Minor:
if (is_deopt) {}
else { }

might be clearer than if !deopt, continue

1185 ↗(On Diff #40461)

minor: reserve the final size

This revision is now accepted and ready to land.Nov 17 2015, 5:49 PM
This revision was automatically updated to reflect the committed changes.
sanjoy marked 4 inline comments as done.