Vector concatenation is cheap as it can be achieved with a zip or mov.
Diff Detail
Event Timeline
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
---|---|---|
2105 | I think that getVectorNumElements will assert if SubLT.second is not a vector, so the additional isVector() call below is redundant here. If you specifically want to guard against the case where it isn't a vector it's probably better to write this as: if (SubLT.second.isVector() && (Index + SubLT.second.getVectorNumElements()) == NumElts) |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
---|---|---|
2105 | Ah yeah, thanks, you're right about that. I was a bit too enthusiastic hoisting that getVectorNumElements() out of the if. Will fix this. Will also fix the failing sve-intrinsics.ll that I forgot to update. |
Can we also add some more tests, for things that are insert_subvector's but not just the simple concat's?
Yeah, I have tried, but unsuccessfully. The shuffle cost for a SK_InsertSubvector is guarded by isInsertSubvectorMask():
if (Shuffle->isInsertSubvectorMask(NumSubElts, SubIndex)) return TargetTTI->getShuffleCost( TTI::SK_InsertSubvector, VecTy, Shuffle->getShuffleMask(), SubIndex, FixedVectorType::get(VecTy->getScalarType(), NumSubElts));
And this isInsertSubvectorMask() seems to only return true for subverter masks that are identity masks, so I haven't been able to trigger this with other masks.
- fixed the if-statement,
- fixed sve-intrinsics.ll.
See my previous comment about subvector insertions that are not concats; haven't been able to trigger that and find test cases for that.
llvm/test/Analysis/CostModel/AArch64/sve-intrinsics.ll | ||
---|---|---|
7 | I have just reran the script, but this is the only real change in this test. |
llvm/test/Analysis/CostModel/AArch64/sve-intrinsics.ll | ||
---|---|---|
7 | The cost for this looks very optimistic judging by the codegen as I can see around 30 instructions generated! Not sure why this has changed as I'd have expected an assert to fire in getVectorNumElements for scalable vectors. |
llvm/test/Analysis/CostModel/AArch64/sve-intrinsics.ll | ||
---|---|---|
7 |
Because of this: unsigned getVectorNumElements() const { // TODO: Check that this isn't a scalable vector. return getVectorMinNumElements(); } So it is happily returning 4 for this case. :-( I think I will add a check for scalable vectors and a TODO for this. |
This would count too %X = shufflevector <8 x i16> undef, <8 x i16> undef, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 8, i32 9>. It has an identity for the first vector, with the second inserted into it.
I think the condition can be more specific to a concat if those are the cases that are really cheap. Two 64bit vectors combined together into a 128bit vector.
Nothing has happened with this patch since last August, so I'm resigning as a reviewer for now!
I think that getVectorNumElements will assert if SubLT.second is not a vector, so the additional isVector() call below is redundant here. If you specifically want to guard against the case where it isn't a vector it's probably better to write this as: