This is an archive of the discontinued LLVM Phabricator instance.

[TTI] NFC: Change getTypeLegalizationCost to return InstructionCost.
ClosedPublic

Authored by dfukalov on Apr 29 2021, 6:33 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

Diff Detail

Event Timeline

dfukalov created this revision.Apr 29 2021, 6:33 AM
dfukalov requested review of this revision.Apr 29 2021, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 6:33 AM

@sdesmalen, I remember about adding invalid cost return for TypeScalarizeScalableVector, but want to leave this change NFC.

This revision is now accepted and ready to land.Apr 29 2021, 8:51 AM
sdesmalen added inline comments.Apr 29 2021, 8:56 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
756

AIUI, the LLVM coding standard favours std::pair<InstructionCost, MVT> over auto, since it's not obvious from the call to TLI->getTypeLegalizationCost what the returned type is.

(same for other instances in this file)

llvm/lib/Target/X86/X86TargetTransformInfo.cpp
3056

I would avoid introducing these changes in this patch, as it just adds unnecessarily to the diff

kparzysz added inline comments.Apr 29 2021, 9:25 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
756

I was going to make a similar comment, but the actual type is LegalizeCost (which happens to be the pair). Since LegalizeCost is defined in TargetLoweringBase, it would have to be TargetLowering::LegalizeCost everywhere. Since it's fairly long, I thought that const auto would be ok, but I agree that spelling out the type would make the code a bit clearer.

So what do you think, should I use TargetLoweringBase::LegalizeCost as type everywhere (and it doesn't seem clearer than auto) or just remove the new type and use std::pair<InstructionCost, MVT> everywhere?

I just saw a number auto LT = TLI->getTypeLegalizationCost() in AArch64 backend and decided it is ok.

So what do you think, should I use TargetLoweringBase::LegalizeCost as type everywhere (and it doesn't seem clearer than auto) or just remove the new type and use std::pair<InstructionCost, MVT> everywhere?

I just saw a number auto LT = TLI->getTypeLegalizationCost() in AArch64 backend and decided it is ok.

I agree with @kparzysz that the expanded form makes the code more readable, so that would have my preference in general. For this patch it's probably best to just follow precedent as to what was used before, i.e. cases that used std::pair<unsigned, MVT> should be changed to std::pair<InstructionCost, MVT>, and all cases that were already using auto to be left unchanged.

For this patch it's probably best to just follow precedent as to what was used before, i.e. cases that used std::pair<unsigned, MVT> should be changed to std::pair<InstructionCost, MVT>

I disagree with this part. If there is a name for this type then we should use it, instead of repeating the definition. Either TargetLoweringBase::LegalizeCost or TargetLowering::LegalizeCost.

For this patch it's probably best to just follow precedent as to what was used before, i.e. cases that used std::pair<unsigned, MVT> should be changed to std::pair<InstructionCost, MVT>

I disagree with this part. If there is a name for this type then we should use it, instead of repeating the definition. Either TargetLoweringBase::LegalizeCost or TargetLowering::LegalizeCost.

The type is actually added in the patch so if we'll leave std::pair, it is not needed.

The type is actually added in the patch so if we'll leave std::pair, it is not needed.

Alright then. Sounds like a plan.

dfukalov updated this revision to Diff 341914.Apr 30 2021, 8:07 AM

Leaving std::pair usage instead of new type and auto.

dfukalov updated this revision to Diff 341917.Apr 30 2021, 8:16 AM

Removed two blank lines added accidentally.

Thanks for your effort to revert the auto changes. Just have two nits, other than that the patch looks good to me!

llvm/include/llvm/CodeGen/BasicTTIImpl.h
360–363

nit: given that InstructionCost is signed (it uses int as the CostType), would it be an idea to add an assert that the value is >= 0?

llvm/lib/Target/X86/X86TargetTransformInfo.cpp
3165–3168

nit: Maybe just use int instead of const InstructionCost::CostType? (+ assert that the value is >= 0)

dfukalov updated this revision to Diff 342004.Apr 30 2021, 12:09 PM

Addressed comments.

sdesmalen accepted this revision.Apr 30 2021, 12:31 PM
This revision was landed with ongoing or failed builds.Apr 30 2021, 12:52 PM
This revision was automatically updated to reflect the committed changes.