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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau 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>>
----------------
sdesmalen wrote:
> 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();
I don't think two template instantiations is that much overhead. I'm trying to avoid going down the slippery slope of having InstructionCost.h be filled with thousands of operator overloads. I see a dystopian future where we have overloads of //some of// the operators for floating point types, cl::opt of floats and ints, Optional of floats and ints, APFloat, APInt, Constant * floats and ints, and gosh knows what else, all only used in one or two places. In this world, every time somebody hits an edge case, they go and add an operator overload to InstructionCost, and maybe they do it right, and maybe they don't.

I would think that 99% of the usages of the operators are between int and InstructionCost, or between InstructionCost and InstructionCost. If we provide a map function, we can give people a way to handle these edge cases that isn't too much more verbose but keeps the interface clean. I want it to be the case where InstructionCost.h hits a steady state where it just provides all the tools anybody could ever need. Realistically, InstructionCost is just Optional<int> with convenient operator overloads for arithmetic. map is the only function (and getValueOr, but that's just gravy), from the interface of Optional<int> that we don't have.

If we must go down this route, I'd like for you to add all the operators all at once. But I don't think we need that; we really shouldn't be doing lots of floating point math anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97382