Page MenuHomePhabricator

NFC: Migrate PartialInlining to work on InstructionCost
ClosedPublic

Authored by sdesmalen on Feb 24 2021, 7:05 AM.

Details

Summary

This patch migrates cost values and arithmetic to work on InstructionCost.
When the interfaces to TargetTransformInfo are changed, any InstructionCost
state will propagate naturally.

See this patch for the introduction of the type: https://reviews.llvm.org/D91174
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146408.html

Diff Detail

Event Timeline

sdesmalen created this revision.Feb 24 2021, 7:05 AM
sdesmalen requested review of this revision.Feb 24 2021, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 7:05 AM
ctetreau added inline comments.Feb 24 2021, 9:56 AM
llvm/include/llvm/Support/InstructionCost.h
116

why is this necessary?

sdesmalen added inline comments.Feb 25 2021, 3:05 AM
llvm/include/llvm/Support/InstructionCost.h
116

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.

ctetreau added inline comments.Feb 25 2021, 7:23 AM
llvm/include/llvm/Support/InstructionCost.h
116

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.

sdesmalen added inline comments.Feb 25 2021, 7:56 AM
llvm/include/llvm/Support/InstructionCost.h
116

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.

ctetreau added inline comments.Feb 26 2021, 12:20 PM
llvm/include/llvm/Support/InstructionCost.h
116

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)

sdesmalen added inline comments.Mar 1 2021, 5:57 AM
llvm/include/llvm/Support/InstructionCost.h
116

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();
ctetreau added inline comments.Mar 2 2021, 10:05 AM
llvm/include/llvm/Support/InstructionCost.h
116

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.

sdesmalen updated this revision to Diff 328212.Mar 4 2021, 9:31 AM

Replaced overloaded operator with InstructionCost::map() function.

sdesmalen added inline comments.Mar 4 2021, 9:40 AM
llvm/include/llvm/Support/InstructionCost.h
116

You were right about this being an edge case, this was actually the only instance that broke when I removed the operator.
I agree with you now. Removing these operators means that all operations on InstructionCost are integer-based, and if someone needs to handle an edge case like this FP -> Int conversion, map provides the functionality. I've updated the patch to reflect this. Thank you for the suggestion!

Aside from the function signature for InstructionCost::map, this patch looks good to me.

llvm/include/llvm/Support/InstructionCost.h
21

if map takes a template parameter for the function type, then this should be unnecessary.

204–208

Having the function type be a template parameter (as it is defined in Optional.h) is preferred because lambda types can be directly used without the indirection of assigning them to a std::function. It also avoids having to #include <functional>

204–208

also, I believe map can be const.

sdesmalen updated this revision to Diff 328473.Mar 5 2021, 4:25 AM

Use template cost function for InstructionCost::map.

sdesmalen marked 7 inline comments as done.Mar 5 2021, 4:31 AM
sdesmalen added inline comments.
llvm/include/llvm/Support/InstructionCost.h
204–208

Having the function type be a template parameter (as it is defined in Optional.h) is preferred because lambda types can be directly used without the indirection of assigning them to a std::function. It also avoids having to #include <functional>

Okay, I wasn't sure if it was better to limit the signature of Function by specifying it would take/return a CostType by using std::function. Any issues resulting from the lambda return the wrong value type probably show up when calling the constructor of InstructionCost.

ctetreau added inline comments.Mar 8 2021, 9:26 AM
llvm/include/llvm/Support/InstructionCost.h
204–208

Yeah, the function object has to take one argument that coerces to CostType, and return some type that coerces to InstructionCost, so it should be safe. I guess it would be nice to constrain the signature to prevent people from thinking they need to return InstructionCost and invoking the copy constructor for no reason, but this version is safer to call in a hot inner loop which I think most C++ programmers would prefer.

llvm/lib/Transforms/IPO/PartialInlining.cpp
1363

this looks like a behavioral change. Please just assert that the costs are valid.

sdesmalen updated this revision to Diff 330262.Mar 12 2021, 8:54 AM
sdesmalen marked an inline comment as done.

Removed functional change and replaced it by an assert.

paulwalker-arm accepted this revision.Mar 15 2021, 5:25 AM
paulwalker-arm added inline comments.
llvm/lib/Transforms/IPO/PartialInlining.cpp
1361

This comment doesn't make sense anymore given the most recent change. Probably easier to just remove it?

This revision is now accepted and ready to land.Mar 15 2021, 5:25 AM
sdesmalen updated this revision to Diff 330624.Mar 15 2021, 5:50 AM

Removed stale comment.

@ctetreau are you also happy with the patch after the latest changes?

This revision was automatically updated to reflect the committed changes.