Page MenuHomePhabricator

NFC: Change getUserCost to return InstructionCost
ClosedPublic

Authored by sdesmalen on Feb 25 2021, 6:30 AM.

Details

Summary

This patch migrates the TTI cost interfaces to return an InstructionCost.

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

Depends on D97382

Diff Detail

Event Timeline

sdesmalen created this revision.Feb 25 2021, 6:30 AM
sdesmalen requested review of this revision.Feb 25 2021, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 6:30 AM
paulwalker-arm added inline comments.Feb 25 2021, 9:41 AM
llvm/include/llvm/Support/InstructionCost.h
213–221 ↗(On Diff #326373)

Are these strictly necessary? I'm wondering if there's just the odd comparison somewhere whose operands can be switched.

Or is it the case that these are a temporary measure that will be removed once the current LHS becomes an InstructionCost? If so I think it's worth adding a //TODO: Remove me when...

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
2168

More of an open question but is this initialisation required/preferred? Or to put another way, what is the expected meaning of InstructionCost()? Invalid? Zero?

sdesmalen updated this revision to Diff 326641.Feb 26 2021, 3:08 AM
sdesmalen marked 2 inline comments as done.

Removed operator== and operator!=.

sdesmalen added inline comments.Feb 26 2021, 3:09 AM
llvm/include/llvm/Support/InstructionCost.h
213–221 ↗(On Diff #326373)

I've removed them, it was indeed the odd comparison in InlineCost.cpp.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
2168

The default InstructionCost() initialises with a valid 0. I would prefer to remove that constructor in favour of initializing it with an explicit value (when this is used for an accumulating cost value) or Invalid when used as 'uninitialized'. The only issue is that this requires some more changes, because it's used in a std::pair<> in e.g. LoopVectorize.cpp, where it needs a default constructor.

Does changing the -1's to invalid's constitute a functional change? Are they always checked for? If they are just blindly added to other costs, then they would result in valid costs becoming invalid costs.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
2168

I don't like the default constructor either. I think a default constructed InstructionCost should be invalid.

dmgreen added inline comments.Feb 28 2021, 10:42 AM
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
1063

This seems strange. Does it need to return an invalid cost? Or can it just return a cost of 1 for all cost kinds?

sdesmalen updated this revision to Diff 327092.Mar 1 2021, 6:15 AM

Removed possibly functional changes (-1 -> Invalid) from this patch.

Does changing the -1's to invalid's constitute a functional change? Are they always checked for? If they are just blindly added to other costs, then they would result in valid costs becoming invalid costs.

In TargetTransformInfo::getUserCost (in TargetTransformInfo.cpp) it has the following check:

InstructionCost Cost = TTIImpl->getUserCost(U, Operands, CostKind);
assert((CostKind == TTI::TCK_RecipThroughput || Cost >= 0) &&
       "TTI should not produce negative costs!");

By returning Invalid the assert no longer fails but instead Invalid is propagated, which you are right is a functional change.
I'll revert to returning -1 instead of Invalid, and will leave that to a separate (non-NFC) patch.

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
1063

For these patches, I haven't really tried to understand the reason behind why things have been cost-modelled the way they have, and have instead focussed on the interface-change alone. I agree returning -1 seems odd, I don't know why this cannot be costed.

ctetreau accepted this revision.Mar 2 2021, 9:33 AM

Assuming everybody else is satisfied, LGTM

This revision is now accepted and ready to land.Mar 2 2021, 9:33 AM
paulwalker-arm accepted this revision.Mar 2 2021, 3:59 PM
This revision was landed with ongoing or failed builds.Wed, Mar 31, 2:14 AM
This revision was automatically updated to reflect the committed changes.