Skip to content

[RISCV] Add cost for @llvm.experimental.vp.splat #117313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 25, 2024
Merged

[RISCV] Add cost for @llvm.experimental.vp.splat #117313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ca219a
Precommit tests
lukel97 Nov 22, 2024
eaac532
[RISCV] Add cost for @llvm.experimental.vp.splat
lukel97 Nov 22, 2024
b50a648
Add tests for fp and i1, return invalid for i1 vectors
lukel97 Nov 22, 2024
ec89b5c
Explicitly use VFMV_V_F
lukel97 Nov 22, 2024
2d02fa5
Return invalid without V instructions
lukel97 Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,16 @@ RISCVTTIImpl::getIntrinsicInstrCost(const IntrinsicCostAttributes &ICA,
return getCmpSelInstrCost(Instruction::Select, ICA.getReturnType(),
ICA.getArgTypes()[0], CmpInst::BAD_ICMP_PREDICATE,
CostKind);
case Intrinsic::experimental_vp_splat: {
auto LT = getTypeLegalizationCost(RetTy);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing the hasV check, and it looks like this is only handling integer (not FP).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also the i1 case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no V instructions then getTypeLegalizationCost will return an invalid type legalization cost for scalable vectors, or a scalar legalized type for fixed vectors, both of which will end up in invalid cost. We could be more explicit about this though?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed vector case will probably end up passing a scalar type to the getRISCVInstructionCost routine which I don't think is what we want. Please add the same type of guard we see elsewhere in this switch for the moment, we can come back and revisit in batch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I've made it return invalid rather than falling out of the switch since we can't scalarize scalable nor fixed versions of this intrinsic yet

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up, can you add handling for fixed vectors (only) in BasicTTI via getShuffleCost? We should be modeling the scalarization here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look into this, but ended up in a bit of a rabbit hole. The default BasicTTIImpl scalarization implementation calls into the InsertElement/ExtractElement cost, but currently we return a cost of 0 for constant indices which at least isn't true for the inserts generated from a scalarized splat shuffle, for example.

However changing the InsertElement/ExtractElement cost seems somewhat sensitive, see #67334. I think we may want to make the cost "dumber", e.g. just a single scalar load/store for any insert or extract, but we would need to double check this doesn't cause excessive unrolling.

This is more complicated than what I first thought it would be, so leaving a note here to maybe return to later.

// TODO: Lower i1 experimental_vp_splat
if (!ST->hasVInstructions() || LT.second.getScalarType() == MVT::i1)
return InstructionCost::getInvalid();
return LT.first * getRISCVInstructionCost(LT.second.isFloatingPoint()
? RISCV::VFMV_V_F
: RISCV::VMV_V_X,
LT.second, CostKind);
}
case Intrinsic::vp_reduce_add:
case Intrinsic::vp_reduce_fadd:
case Intrinsic::vp_reduce_mul:
Expand Down
Loading
Loading