[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:
> > > > 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97382