[PATCH] D79215: [llvm][NFC] Inliner: simplify inlining decision logic

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D79215#2016450 <https://reviews.llvm.org/D79215#2016450>, @dblaikie wrote:

> In D79215#2016106 <https://reviews.llvm.org/D79215#2016106>, @davidxl wrote:
>
> > 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79215/new/

https://reviews.llvm.org/D79215