This is an archive of the discontinued LLVM Phabricator instance.

Refactor inline costs analysis by removing the InlineCostAnalysis class
ClosedPublic

Authored by eraman on Dec 21 2015, 1:26 PM.

Details

Summary

InlineCostAnalysis is an analysis pass without any need for it to be one. Once it stops being an analysis pass, it doesn't maintain any useful state and the member functions inside can be made free functions. NFC.

(Note: InlineCost.h has the following comment for getInlineCost, a member function of InlineCostAnalysis:

  • // Note: This is used by out-of-tree passes, please do not remove without
  • // adding a replacement API.

I am not sure what this means, but this refactoring makes this into a free function with extra parameters)

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 43397.Dec 21 2015, 1:26 PM
eraman retitled this revision from to Refactor inline costs analysis by removing the InlineCostAnalysis class.
eraman updated this object.
eraman added a reviewer: chandlerc.
eraman set the repository for this revision to rL LLVM.
eraman added subscribers: llvm-commits, davidxl.
chandlerc edited edge metadata.Dec 21 2015, 3:10 PM

Thanks Easwaran, this seems like a really nice clean up. Just one minor fix needed below...

lib/Transforms/IPO/InlineSimple.cpp
57–59 ↗(On Diff #43397)

These are actually expensive operations. Instead of doing this once per getInlineCost, it would be better to cache them in members of the actual pass (either the SimpleInliner or Inliner depending on what is cleanest).

108–111 ↗(On Diff #43397)

Hmm, this makes me believe that at least the base class should manage calling 'getAnalysis' for AssumptionCacheTracker, and probably this class should do so for TargetTransformInfo...

eraman added inline comments.Dec 21 2015, 3:24 PM
lib/Transforms/IPO/InlineSimple.cpp
57–59 ↗(On Diff #43397)

Ok.

108–111 ↗(On Diff #43397)

Not fully sure what you mean here. The base class calls getAnalysis for AssumptionCacheTracker, and you want to cache the value in the base class so that it could be used in derived class's InlineCost?

chandlerc added inline comments.Dec 21 2015, 3:49 PM
lib/Transforms/IPO/InlineSimple.cpp
108–111 ↗(On Diff #43397)

Yea, just make a protected member that the derived class can access when calling getInlineCost so it doesn't have to call getAnalysis again.

(The fact that this is somewhat magical is one of many reasons why I don't really like the pattern SimpleInliner AlwaysInliner use of subclassing a fully formed pass, but I think that's a much bigger refactoring yak to shave)

eraman updated this revision to Diff 43425.Dec 21 2015, 5:33 PM
eraman edited edge metadata.

Mostly nits...

lib/Analysis/InlineCost.cpp
1322 ↗(On Diff #43397)

Rather than putting all of this in the llvm namespace, you can just use the qualified function name "llvm::getInlineCost" in the definition.

lib/Transforms/IPO/InlineSimple.cpp
55–56 ↗(On Diff #43425)

Why add this code?

65 ↗(On Diff #43425)

You can actually make TTI the member instead of the wrapper pass.

104–105 ↗(On Diff #43425)

You want to replace this on every run rather than checking it on each run. Look for other places where we cache a TTI pointer. You should also leave it uninitialized in the constructor so we get an MSan error if we reach code without the run call happening first.

lib/Transforms/IPO/Inliner.cpp
477–478 ↗(On Diff #43425)

Same comment as above, just overwrite the pointer each time.

eraman marked an inline comment as done.Dec 22 2015, 10:56 AM
eraman added inline comments.
lib/Transforms/IPO/InlineSimple.cpp
55–56 ↗(On Diff #43425)

Originally, there was a if(Callee) guard before calling the getTTI, but the Callee can never be NULL here. I agree that assert(Callee) is not very useful and will remove this.

65 ↗(On Diff #43425)

I'm confused. Isn't the returned TTI dependent on the function? If I make TTI a member, how do I get the TTI for the callee in getInlineCost without calling getAnalysis on TTI wrapper pass ?

104–105 ↗(On Diff #43425)

I didn't think about the MSan interaction and it now makes sense to remove the initialization and hence can't do the if (!TTIWP)... . I am curious if there is any other reason to avoid this pattern.

chandlerc added inline comments.Dec 22 2015, 11:37 AM
lib/Transforms/IPO/InlineSimple.cpp
65 ↗(On Diff #43425)

Quite right. Sorry for the noise!

104–105 ↗(On Diff #43425)

It doesn't happen much in-tree, but the pass manager supports running multiple modules through a single pass instance, and it is conceivable that they would have different TTI wrapper passes. Not likely, and not practically reproducible, but the pattern is designed to handle cases where the analysis might not be valid to cache from run to run.

eraman updated this revision to Diff 43471.Dec 22 2015, 12:15 PM

Address Chandler's comments

chandlerc accepted this revision.Dec 28 2015, 9:50 AM
chandlerc edited edge metadata.

This LGTM, feel free to submit with the argument naming tweak below.

However, this naming tweak raises an interesting question for me. This should *not* be addressed in this patch, but it might be a good thing to put on your queue.

We are passing the *callee* TTI into the inline cost analysis. That doesn't make a lot of sense to me. We're inlining into the *caller*. If there is something *incompatible* about the callee, we should refuse to inline it. If they are compatible, the *caller's* TTI should become dominant. As an example, if we have one function which is marked as optimized for size, but we inline it into code that is not optimized for size, I would expect the inlined body to *not* be optimized for size. I would generally expect the TTI of the call site to determine the cost analysis for inlining because after inlining, the callee is gone. Does that make sense? That will be a non-trivial functional change, so you'll want to benchmark it etc before making it.

Relatedly, I know you're looking heavily at getting our nested call site analysis to be accurate. You should make sure we're considering the possibility of incompatible function attributes that might prevent inlining entirely. I don't think we currently model that correctly, in that the cost analysis skips it but then the inliner itself considers it.

include/llvm/Analysis/InlineCost.h
112 ↗(On Diff #43471)

I would clarify that this is currently the callee's TTI, maybe just by naming it "CalleeTTI".

This revision is now accepted and ready to land.Dec 28 2015, 9:50 AM

This LGTM, feel free to submit with the argument naming tweak below.

However, this naming tweak raises an interesting question for me. This should *not* be addressed in this patch, but it might be a good thing to put on your queue.

We are passing the *callee* TTI into the inline cost analysis. That doesn't make a lot of sense to me. We're inlining into the *caller*. If there is something *incompatible* about the callee, we should refuse to inline it. If they are compatible, the *caller's* TTI should become dominant. As an example, if we have one function which is marked as optimized for size, but we inline it into code that is not optimized for size, I would expect the inlined body to *not* be optimized for size. I would generally expect the TTI of the call site to determine the cost analysis for inlining because after inlining, the callee is gone. Does that make sense?

Yes. We will have to pass both the caller and callee TTIs when I get to implementing estimated speedup that requires computing the cost of the inlined and uninlined versions of the callee.

That will be a non-trivial functional change, so you'll want to benchmark it etc before making it.

Ok.

Relatedly, I know you're looking heavily at getting our nested call site analysis to be accurate. You should make sure we're considering the possibility of incompatible function attributes that might prevent inlining entirely. I don't think we currently model that correctly, in that the cost analysis skips it but then the inliner itself considers it.

Sure.

This revision was automatically updated to reflect the committed changes.

Chandler, do you have more comments?