[PATCH] D97382: NFC: Migrate PartialInlining to work on InstructionCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Support/InstructionCost.h:117
 
+  template <typename T,
+            typename = std::enable_if_t<std::is_floating_point<T>::value>>
----------------
ctetreau wrote:
> sdesmalen wrote:
> > ctetreau wrote:
> > > why is this necessary?
> > I wasn't really sure what type to choose for RHS (`float` or `double`), so I thought I'd just template the type for any floating point type.
> > 
> > The operator is needed for a multiplication in PartialInlining with the options `MinRegionSizeRatio` and `ColdBranchRatio` which in this case are both of type `float`.
> It looks like in all cases where those two options are used, they are immediately static_cast<int>'ed before being assigned to anything. Could you just keep the static cast and not have this operator overload?
> 
> My fear is that we're going to end up with a billion operator overloads that are only used in one or two places.
Just to make sure we're on the same page, the instance for which I added this operator is:
  InstructionCost MinOutlineRegionCost =
      OverallFunctionCost * MinRegionSizeRatio.getValue();
where `OverallFunctionCost` is now an InstructionCost.

In my previous comment I suggested `ColdBranchRatio` was also used in a computation with InstructionCost, but that was not correct (that is the one being static_cast'ed to int).

The alternative way to write this is to check for validity and ask the for the value, multiply with the FP value, and return a new InstructionCost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97382