This patch adds/fixes memory op cost model for SVE with fixed-width
vector.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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; |
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. | |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h | ||
150 | Yes, indeed. Missed that, thank you Sander. | |
llvm/test/Analysis/CostModel/AArch64/mem-op-cost-model.ll | ||
52 | Hi @rengolin you are correct. |
LGTM, thanks for the changes @CarolineConcatto
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
---|---|---|
791 | nit: unrelated change. |
Missing const for Type (because it doesn't modify Ty) and the method itself (because it doesn't modify the TargetTransform object), e.g.