This is an archive of the discontinued LLVM Phabricator instance.

[Inlining] Teach the inliner pass to inline through statepoints.
AbandonedPublic

Authored by sanjoy on Jun 22 2015, 7:15 PM.

Details

Summary

The cost analysis pass does not currently grok statepoints. We only
inline through a gc_statepoint if the call alwaysinline.

Depends on D10632

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 28193.Jun 22 2015, 7:15 PM
sanjoy retitled this revision from to [Inlining] Teach the inliner pass to inline through statepoints..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added reviewers: reames, chandlerc, nlewycky.
sanjoy added a subscriber: Unknown Object (MLST).Jun 22 2015, 7:16 PM

ping, so that llvm-commits sees this (forgot to subscribe llvm-commits when I created the patch).

Please ignore this change. I thought I had run the test suite after this, but I had not and a bunch of test fail with this applied. I'll put up an update and fixed change tomorrow.

sanjoy updated this revision to Diff 28285.Jun 23 2015, 2:15 PM
  • update with fixed patch!
reames requested changes to this revision.Jun 24 2015, 12:00 PM
reames edited edge metadata.
reames added inline comments.
include/llvm/IR/CallSite.h
107

a) comments! both here on getCalledFunction to distinguish
b) naming - "meta" isn't real descriptive. Possible getCalledFunctionUnwrapped?

lib/Transforms/IPO/Inliner.cpp
457

Again, it's really not clear to me that the extra level of abstraction is worthwhile here.

612

Given we already have the getMetaCallDestination routine on callsite, I'm not sure what this extra complexity gains us.

lib/Transforms/Utils/InlineFunction.cpp
165

This looks unrelated?

This revision now requires changes to proceed.Jun 24 2015, 12:00 PM
pgavlin added inline comments.Jun 24 2015, 3:20 PM
include/llvm/IR/CallSite.h
107

This feels a bit ugly to me, esp. since the information about the location of the actual callee in the statepoint argument list is also present as a constant in Statepoint.h. Would it perhaps make sense to split StatepointBase into 2 types--a base type that provides argument accessors and can be included next to CallSite, and a derived type that provides getRelocates()?

Bikeshedding a bit, I'd probably also rename this to getActualCalledFunction() or similar.

lib/Transforms/IPO/Inliner.cpp
139–145

Should this simply be using getMetaCallDestination()?

457–473

This abstraction seems unnecessary: why not just s/CS.getCalledFunction()/CS.getMetaCallDestination()?

sanjoy updated this revision to Diff 28528.Jun 25 2015, 7:15 PM
sanjoy edited edge metadata.
  • remove InlineSite
  • rename the accessor on Statepoint to getTargetFunction. I chose this name because the statepoint documentation uses the term "target"; but more bikeshedding is welcome.
sanjoy added inline comments.Jun 25 2015, 7:18 PM
include/llvm/IR/CallSite.h
107

I think there is scope to (at least) extract out a StatepointOffsets struct with all of the constants encoded as static const ints and include that in CallSite.h as well as Statepoint.h. Do you think something like that will be sufficient?

sanjoy abandoned this revision.Jun 25 2015, 8:01 PM

I've merged D10633 and D10632 into D10758. Their combined size is not too large IMO, and keeping them together lets me avoid adding unneeded complexity via flags like -spp-dont-pre-inline-polls just for testing.