This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]Add memory op cost model for SVE
ClosedPublic

Authored by CarolineConcatto on Nov 6 2020, 8:22 AM.

Details

Summary

This patch adds/fixes memory op cost model for SVE with fixed-width
vector.

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Nov 6 2020, 8:22 AM
rengolin accepted this revision.Nov 6 2020, 8:40 AM

Nice change, simpler to read and easier to get right. LGTM, thanks!

llvm/test/Analysis/CostModel/AArch64/mem-op-cost-model.ll
52

I'm assuming that the idea is that this vector will be NEON and not SVE, so the cost is the same.

llvm/test/Analysis/CostModel/AArch64/scalable-mem-op-cost-model.ll
2

Nit: No empty first line and one empty line between RUN line and first piece of code.

This revision is now accepted and ready to land.Nov 6 2020, 8:40 AM
sdesmalen added inline comments.Nov 6 2020, 9:08 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
753

nit: add whitespace above.

754

You can write isa<FixedVectorType>(Ty) instead of !cast<VectorType>(Ty)->getElementCount().isScalable().

Maybe this is personal preference, but it reads simpler to me if you rewrite the boolean expression to:

return isa<FixedVectorType>(Ty) && !ST->useSVEForFixedLengthVectors();
785

useNeonVector should only return true if Ty is a vector, so you can simplify this expression to:

if (useNeonVector(Ty) &&
   cast<VectorType>(Ty)->getElementType()->isIntegerTy()) {
  ...
}
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
150

Missing const for Type (because it doesn't modify Ty) and the method itself (because it doesn't modify the TargetTransform object), e.g.

bool useNeonVector(const Type *Ty) const;
  • address review's comment
CarolineConcatto retitled this revision from [AArch64]Add memory op cost model for Neon to [AArch64]Add memory op cost model for SVE.Nov 9 2020, 5:43 AM
CarolineConcatto edited the summary of this revision. (Show Details)
CarolineConcatto marked 6 inline comments as done.Nov 9 2020, 5:44 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
754

Thank you for the suggestion. I agree that is clear when working with boolean expression.

785

I think this is fixed.
Just in case, I believe the 8 inside cast<VectorType>(Ty)->getElementType()->isIntegerTy() is not intentional

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
150

Yes, indeed. Missed that, thank you Sander.
I believe it is fixed now.

llvm/test/Analysis/CostModel/AArch64/mem-op-cost-model.ll
52

Hi @rengolin you are correct.
I added an explanation at the beginning of the test.
According to the test useSVEForFixedLengthVectors() all fixed vector width lower than 256 bits will neon isa instead of sve, so the performance for CHECK-NEON and CHECK-SVE-128 should be the same.

sdesmalen accepted this revision.Nov 9 2020, 1:44 PM

LGTM, thanks for the changes @CarolineConcatto

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
791

nit: unrelated change.

This revision was landed with ongoing or failed builds.Nov 11 2020, 4:55 AM
This revision was automatically updated to reflect the committed changes.
CarolineConcatto marked 4 inline comments as done.