[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:
> > > 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.
> Oh, I see the issue. Instead of providing overloads for float, you could add a map function like Optional has:
> 
> ```
>   // F is a function from CostType to CostType
>   template <class Function>
>   InstructionCost map(const Function &F) const {
>     if (isValid()) 
>       return F(*getValue());
>     return getInvalid();
>   }
> ```
> 
> Then you could have:
> 
> ```
> InstructionCost MinOutlineRegionCost = OverallFunctionCost.map(
>         [&](auto OFC) { return OFC * MinRegionSizeRatio; });
> ```
> 
> (I guess you could delegate to Optional<InstructionCost::CostType>::map, but I don't think that would save any keystrokes)
That's an interesting way to write it. Maybe the use of `auto` here would be a bit confusing, as this is a InstructionCost::CostType.

Would writing:
  InstructionCost MinOutlineRegionCost = OverallFunctionCost * MinRegionSizeRatio;
not be more intuitive though?

I guess I'm trying to understand your reasoning for not doing the operator overload. Is that because you don't like the fact that it now needs to instantiate two functions (one for float and one for double, depending on it's use), or are you concerned about the truncation that happens when the result is converted back is unexpected when working on an InstructionCost? If it is the former, then I think in practice there will be at most two overloads that need instantiating, whereas for the latter, I'm not really concerned because all existing code that would use this already assumes truncation.

The getValue() part of `MinRegionSizeRatio.getValue()` is needed because MinRegionSizeRatio is a `cl::opt`, hence:
  InstructionCost MinOutlineRegionCost = OverallFunctionCost * MinRegionSizeRatio.getValue();


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97382