This is an archive of the discontinued LLVM Phabricator instance.

[llvm][NFC] Inliner: simplify inlining decision logic
ClosedPublic

Authored by mtrofin on Apr 30 2020, 4:34 PM.

Details

Summary

shouldInline makes a decision based on the InlineCost of a call site, as well as an evaluation on whether the site should be deferred. This means it's possible for the decision to be not to inline, even for an
InlineCost that would otherwise allow it.

Both uses of shouldInline performed the exact same logic after calling it. In addition, the decision on whether to inline or not was communicated through two values of the Option<InlineCost> return value: None, or an InlineCost evaluating to false.

Simplified by:

  • encapsulating the decision in the return object. The bool it evaluates to communicates unambiguously the decision. The InlineCost is also available.
  • encapsulated the common post-shouldInline code into shouldInline.

Diff Detail

Event Timeline

mtrofin created this revision.Apr 30 2020, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 4:34 PM
mtrofin updated this revision to Diff 261392.Apr 30 2020, 4:47 PM

Removed unnecessary {}

davidxl added inline comments.Apr 30 2020, 4:50 PM
llvm/lib/Transforms/IPO/Inliner.cpp
478

Returning None simplifies the check of the return value a little, but it drops information. Perhaps just return IC in case the user needs to check it in different context?

mtrofin marked an inline comment as done.Apr 30 2020, 5:37 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

That's the thing, the user doesn't use the IC in the 'don't inline' case.

mtrofin marked an inline comment as done.Apr 30 2020, 5:40 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

Or you mean return InlineCost, not Optional<InlineCost> - and fabricate a 'never' if needed?

mtrofin marked 2 inline comments as done.Apr 30 2020, 6:10 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

Actually, looking at it, returning InlineCost (not Optional) won't work, we would equally lose information in the deferred case. I could wrap shouldInline, if we expect other users of it (but seems unnecessarily complex?)

davidxl added inline comments.Apr 30 2020, 8:09 PM
llvm/lib/Transforms/IPO/Inliner.cpp
478

I meant return Optional<InlineCost> -- basically does not change the behavior of shouldInline(..) in terms of what is returned. It is not super important -- just feels like the info is computed, used in place and dropped before returning seems unnecessary.

mtrofin marked an inline comment as done.Apr 30 2020, 9:14 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

The info isn't dropped if the call is inlinable though. Just if it's not - in which case we consume all we need there.

This revision is now accepted and ready to land.Apr 30 2020, 11:33 PM
mtrofin updated this revision to Diff 261459.May 1 2020, 8:21 AM

Alternative that both simplifies uses and keeps InlineCost in all cases.

davidxl added inline comments.May 1 2020, 9:37 AM
llvm/lib/Transforms/IPO/Inliner.cpp
419

Why final? There is no inheritance and virtual function, or do you plan to create a hierarchy later?

mtrofin updated this revision to Diff 261498.May 1 2020, 11:15 AM

shorter name

mtrofin edited the summary of this revision. (Show Details)May 1 2020, 11:17 AM
mtrofin edited the summary of this revision. (Show Details)
mtrofin marked 3 inline comments as done.
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
419

Just to reinforce this is meant to be a simple wrapper. Of course, we can change any of this later.

This looks much better. Perhaps change the name of variable like OIC to reflect it too?

mtrofin marked an inline comment as done.May 1 2020, 11:40 AM
mtrofin updated this revision to Diff 261571.May 1 2020, 4:13 PM

OIC -> InlDec

(didn't want to call it ID because that's confusing - it's not an identifier. InliningDecision is the type name, so not ideal. So settled for InlDec)

InlDec sounds fine.

dblaikie added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

I'd like to push back a bit on this - by the looks of it, shouldInline has two callers, neither of which use the InlineCost if shouldInline returns false - I think it'd probably be best to keep the Optional<InlineCost> return for now, until there's a use case/caller that needs the InlineCost data even when it shouldn't be inlining.

(I have a few minor suggestions for the InliningDecision class, if it remains)

mtrofin marked 2 inline comments as done.May 1 2020, 5:04 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/Inliner.cpp
478

(sorry, submitted before I saw your message, happy to address in post-commit!)

I can see it either way. I think the key thing is collapsing the communication of "should/shouldn't inline", and removing the common subsequent code sequence. Both the original proposal (for which I believe you're advocating), and this updated one, address that.

I'd argue that the new type (latest patch) doesn't add much cognitive overhead, and, while I also prefer not exposing things without a user, this case isn't that severe either (subjective opinion, of course).

mtrofin marked an inline comment as done.May 1 2020, 5:08 PM
dblaikie added inline comments.May 1 2020, 5:28 PM
llvm/lib/Transforms/IPO/Inliner.cpp
478

I think the key thing is collapsing the communication of "should/shouldn't inline", and removing the common subsequent code sequence. Both the original proposal (for which I believe you're advocating), and this updated one, address that.

Oh, for sure - no disagreement there.

I'd argue that the new type (latest patch) doesn't add much cognitive overhead, and, while I also prefer not exposing things without a user, this case isn't that severe either (subjective opinion, of course).

Oh, I don't think it's severe - but I do think it's unnecessary, given the two callers currently don't use the extra information that is conveyed compared to Optional<InlineCost>

@davidxl - could you explain more why you find it to be worthwhile adding this extra API surface in now, rather than waiting until a use-case arises that uses the InlineCost even when "shouldInline" returns (the equivalent of) false?

This revision was automatically updated to reflect the committed changes.

I don't feel strongly to add the new interface -- my original suggestion was to return Optional<InlineCost> without dropping it to None.

On the other hand, I do like the new return type though -- using Optional<..> and let client fiddle with hasValue and getValue to figure out the inline decisions is not friendly. Having a new return type does make the user code cleaner.

Having said that, I don't mind either way.

I don't feel strongly to add the new interface -- my original suggestion was to return Optional<InlineCost> without dropping it to None.

Hmm, I'm not sure I follow - use an Optional<InlineCost> return type, but never return the None value - what would be the purpose of the Optional<> in that case? (& I guess in that case you'd need to collapse both "do not inline" results into the InlineCost "Never" result? (at that point I'd suggest using InlineCost alone, without any extra boolean (either through Optional or InliningDecision) - which wouldn't be the worst thing, since InlineCost already has such a "do not inline" state, that could be used directly)

On the other hand, I do like the new return type though -- using Optional<..> and let client fiddle with hasValue and getValue to figure out the inline decisions is not friendly.

Optional<T> is pretty common across the LLVM codebase & other C++ usage (in the form of the C++17 std::optional). hasValue/getValue are quite uncommon ways to interact with it - it's far more common to use the boolean testability (same as InliningDecision) of Optional, and usually the dereference operator (similar to llvm's Expected<T> for instance - which, rather than just providing the boolean "false" value when the T is not present, provides a more expressive Error value - but using a similar API, boolean testable and dereference for access to the underlying T object):

Optional<InlineCost> OIC = shouldInline(...);
if (!OIC)
  continue;
...
inlineCostStr(*OIC);
...
emitInlinedInto(..., *OIC);

Having a new return type does make the user code cleaner.

I'm curious to better understand the cleanliness aspect you're driving at here - if you could provide a small example/compare/contrast, perhaps?

Having said that, I don't mind either way.

I think based on the above, I wouldn't mind either: Optional<InlineCost> as it was before, or perhaps simpler, InlineCost without any wrapper, using NeverInline to communicate the instruction to not inline here.

I don't feel strongly to add the new interface -- my original suggestion was to return Optional<InlineCost> without dropping it to None.

Hmm, I'm not sure I follow - use an Optional<InlineCost> return type, but never return the None value - what would be the purpose of the Optional<> in that case? (& I guess in that case you'd need to collapse both "do not inline" results into the InlineCost "Never" result? (at that point I'd suggest using InlineCost alone, without any extra boolean (either through Optional or InliningDecision) - which wouldn't be the worst thing, since InlineCost already has such a "do not inline" state, that could be used directly)

On the other hand, I do like the new return type though -- using Optional<..> and let client fiddle with hasValue and getValue to figure out the inline decisions is not friendly.

Optional<T> is pretty common across the LLVM codebase & other C++ usage (in the form of the C++17 std::optional). hasValue/getValue are quite uncommon ways to interact with it - it's far more common to use the boolean testability (same as InliningDecision) of Optional, and usually the dereference operator (similar to llvm's Expected<T> for instance - which, rather than just providing the boolean "false" value when the T is not present, provides a more expressive Error value - but using a similar API, boolean testable and dereference for access to the underlying T object):

Optional<InlineCost> OIC = shouldInline(...);
if (!OIC)
  continue;
...
inlineCostStr(*OIC);
...
emitInlinedInto(..., *OIC);

Having a new return type does make the user code cleaner.

I'm curious to better understand the cleanliness aspect you're driving at here - if you could provide a small example/compare/contrast, perhaps?

Having said that, I don't mind either way.

I think based on the above, I wouldn't mind either: Optional<InlineCost> as it was before, or perhaps simpler, InlineCost without any wrapper, using NeverInline to communicate the instruction to not inline here.

I assume when you say "Optional<InlineCost> as it was before", you're referring to "before, in the first iteration of the patch", correct?

I don't feel strongly to add the new interface -- my original suggestion was to return Optional<InlineCost> without dropping it to None.

Hmm, I'm not sure I follow - use an Optional<InlineCost> return type, but never return the None value - what would be the purpose of the Optional<> in that case? (& I guess in that case you'd need to collapse both "do not inline" results into the InlineCost "Never" result? (at that point I'd suggest using InlineCost alone, without any extra boolean (either through Optional or InliningDecision) - which wouldn't be the worst thing, since InlineCost already has such a "do not inline" state, that could be used directly)

On the other hand, I do like the new return type though -- using Optional<..> and let client fiddle with hasValue and getValue to figure out the inline decisions is not friendly.

Optional<T> is pretty common across the LLVM codebase & other C++ usage (in the form of the C++17 std::optional). hasValue/getValue are quite uncommon ways to interact with it - it's far more common to use the boolean testability (same as InliningDecision) of Optional, and usually the dereference operator (similar to llvm's Expected<T> for instance - which, rather than just providing the boolean "false" value when the T is not present, provides a more expressive Error value - but using a similar API, boolean testable and dereference for access to the underlying T object):

Optional<InlineCost> OIC = shouldInline(...);
if (!OIC)
  continue;
...
inlineCostStr(*OIC);
...
emitInlinedInto(..., *OIC);

Having a new return type does make the user code cleaner.

I'm curious to better understand the cleanliness aspect you're driving at here - if you could provide a small example/compare/contrast, perhaps?

Having said that, I don't mind either way.

I think based on the above, I wouldn't mind either: Optional<InlineCost> as it was before, or perhaps simpler, InlineCost without any wrapper, using NeverInline to communicate the instruction to not inline here.

I assume when you say "Optional<InlineCost> as it was before", you're referring to "before, in the first iteration of the patch", correct?

Yep!

I believe there are other paths in the code that computes to None thus the use of Optional<..>. What I meant is the new dropping to None in the original version of the patch. Never is used for noinline attribute so we don't want to convert other do not inline into it.

Having a new wrapper class for 2 uses do look like a borderline overkill -- so revert to the original version is fine to me.

I believe there are other paths in the code that computes to None thus the use of Optional<..>. What I meant is the new dropping to None in the original version of the patch. Never is used for noinline attribute so we don't want to convert other do not inline into it.

Oh! I see, you preferred the original version that differentiated between the two kinds of "no, don't inline" responses, as the original code did (the original code needed that differentiation to handle two different remarks - but now the new code doesn't, because the remark handling is internal - but I understand your goal better now, that you wanted to communicate those two states even though they are no longer/not currently needed to be differentiated by callers at the moment). My take on it was that, now that callers don't (currently) need to differentiate, it'd be simpler if the interface didn't expose that difference at all.

Having a new wrapper class for 2 uses do look like a borderline overkill -- so revert to the original version is fine to me.

OK - in any case, I do appreciate the conversation/better understanding your perspective.

yes, that captured what I meant.

yes, that captured what I meant.

So I have a conclusion, do I:

  • do nothing, or
  • create a new patch as per the first version of this patch here (or revert and do that?)

Thanks!

yes, that captured what I meant.

So I have a conclusion, do I:

  • do nothing, or
  • create a new patch as per the first version of this patch here (or revert and do that?)

Thanks!

If it's OK with you - could you commit the Optional version (like the first version of your patch)? no need to send it for review, you can commit it directly. But if you can follow-up here to mention the commit hash when it's done, that'd be great!