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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:457
+    setInlineRemark(CB, inlineCostStr(IC));
+    return None;
   }
----------------
mtrofin wrote:
> dblaikie wrote:
> > mtrofin wrote:
> > > davidxl wrote:
> > > > mtrofin wrote:
> > > > > mtrofin wrote:
> > > > > > mtrofin wrote:
> > > > > > > davidxl wrote:
> > > > > > > > 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?
> > > > > > > That's the thing, the user doesn't use the IC in the 'don't inline' case.
> > > > > > Or you mean return InlineCost, not Optional<InlineCost> - and fabricate a 'never' if needed?
> > > > > 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?)
> > > > 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.
> > > 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.
> > 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)
> (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).
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79215