This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add cost model for vector insert/extract element.
ClosedPublic

Authored by jacquesguan on Aug 31 2022, 2:29 AM.

Details

Summary

This patch adds cost model for vector insert/extract element instructions. In RVV, we could use vector scalar move instruction to insert or extract the first element, and use vslide to move it. But for mask vector or i64 vector in i32 target, we need special instructions to make it.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 31 2022, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 2:29 AM
jacquesguan requested review of this revision.Aug 31 2022, 2:29 AM
reames requested changes to this revision.Aug 31 2022, 2:59 PM

Generally quite reasonable, a couple comments to address before a likely LGTM.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
768

This routine is used for both insertelement and extractelement. Your comments apply only to extract. I think you're either a) missing a opcode early return, or b) need to give corresponding insert comments.

775

This line is interesting as it implies we should probably have a test check configuration for cases where the vector type is scalarized. Can you add a run line (precommit it please) which *doesn't* have float, but does have vector? Just to make sure we have this case exercised.

Hm, actually, thinking about this. There may be an interesting case here. What happens if we're costing a scalable vector which can't be legalized via scalarizing? Digging through the code, it looks like we return LT.first as invalid in that case. I think we need to propagate that here to be safe.

779

Ok, the following is an *optional* suggestion. Feel free to include it or ignore.

This might be more readable if structured as:
InstructionCost SlideDownCost = Index == 0 ? 0: 1;
InstructionCost MatCost = (your two special cases)
return 1 + SlideDownCost + MatCost.

This revision now requires changes to proceed.Aug 31 2022, 2:59 PM

Address comment.

jacquesguan added inline comments.Sep 1 2022, 2:18 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
768

Thanks, I missed some special cases of insertelement, already added.

775

Thanks for pointing this. If a scalable vector type is not supported in the target, getTypeLegalizationCost would return {InvalidCost, itself}. I missed this case. Now I add an early exit to make sure returning InvalidCost.

reames requested changes to this revision.Sep 1 2022, 8:00 AM

Adding the insertelement handling caused a lot more test diffs. For ease of review, would you mind restricting this patch to extractelement and then doing insertelement in a follow up patch?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
774

I think you're missing a return here.

818

For a follow up patch...

I think we can do better here using logical ops on the mask vector. Haven't fully worked through the sequence, but generating a mask with one bit set is fairly cheap, and then we're just doing bit toggles.

855

I think we're missing something here. Specifically, what is the appropriate cost for an illegally typed vector which has been split? Don't we need to account for the cost of figuring out which sub-vector to extract from/insert into? For constants, this should fold away, but for a non-constant index doesn't this require some logic?

This revision now requires changes to proceed.Sep 1 2022, 8:00 AM

Adding the insertelement handling caused a lot more test diffs. For ease of review, would you mind restricting this patch to extractelement and then doing insertelement in a follow up patch?

The reason why other test cases affected is that some caculation of other instructions' cost depends on the cost of insertelement and extractelement. It my mistake to forget upload them before. So splitting this revision would be not help for reviewing.

jacquesguan added inline comments.Sep 1 2022, 8:18 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
818

I think you mean we could improve the codegen of insertelemet of mask vector.
I do have a revision about fixed mask insertelement https://reviews.llvm.org/D119115, but it is not approved now.
About your comment, I don't think out how to build a scalable mask vector with only a given index set now, which instructions we should use to make it?

Add consider of splitted vector type.

jacquesguan added inline comments.Sep 2 2022, 1:53 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
855

I missed considerring the case of splitted vector. In this case, we would use stack store/load to make it. I added it now.

reames accepted this revision.Sep 12 2022, 12:19 PM

LGTM with one required change made. If you disagree with that change, or it doesn't work for some reason, further discussion is needed.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
787

I don't follow the added code here.

For purposes of driving this review forward, can you split this bit into it's own patch? For this patch, do the following:

if (!isTypeLegal(Val->getType())
  return BaseT::getVectorInstrCost(Opcode, Val, Index);
This revision is now accepted and ready to land.Sep 12 2022, 12:19 PM

Address comment and rebase main.

reames accepted this revision.Sep 13 2022, 8:12 AM

LGTM per previous comment.

This revision was landed with ongoing or failed builds.Sep 13 2022, 8:10 PM
This revision was automatically updated to reflect the committed changes.