[PATCH] D105349: [llvm][Inline] Add interface to return cost-benefit stuff

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineCost.h:273
 
+Optional<std::pair<APInt, APInt>> getInlineCostBenefitPair(
+    CallBase &Call, const InlineParams &Params, TargetTransformInfo &CalleeTTI,
----------------
why do you need the 2 of them? (I don't see a user for either... well, ok, the second calls the first, but other than that)

I think it would be good to hook up to where you want to use this. In particular, if the user will always want the InlineCost, too, it wouldn't be ideal if the callsite analysis happened twice (once for InlineCost, once for the cost benefit pair). So one way you could expose the cost/benefit pair is by having it as a field in the InlineCost; at that point, too, you could either just have the 'cost' and 'benefit' as fields of InlineCost, or see if the struct design I mention below makes more sense.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:495
+  // The cost-benefit pair computed by cost-benefit analysis.
+  std::pair<APInt, APInt> CostBenefitPair;
+
----------------
Nit: the problem with std::pair where both elements are of the same type is that it's not clear to the reader which is which, without reading comments - which is added work (also comments go out of sync eventually).

Alternative: define a little struct with named fields, makes it all crystal-clear; additionally, when using an IDE, autocompletion makes it very easily discoverable.

Even better alternative, from maintainability perspective: define the struct, and make the fields const and set them through the ctor. So then a reader understands that the object, once constructed, won't mutate, so it removes a concern from understanding the code (basically, the values could only have come from when it was constructed, so less code to chase around)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105349