Since shufflevector supports scalable vector in broadcast mode, it
should be handled as well in getShuffleCost. This patch does this which fixes
assertion in https://github.com/llvm/llvm-project/issues/51550,
TestPlan: check-llvm
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
It's fine to have a vectorizer test, but it would be better to have a more direct test of the cost model itself.
I think something like this can check for the crash (we could add variations to see if the cost is actually accurate - see /llvm/test/Analysis/CostModel/AArch64/sve-shuffle-broadcast.ll for an existing test model):
; RUN: opt -cost-model -analyze -mtriple=riscv64 -mattr=+m,+experimental-v -scalable-vectorization=on < %s | FileCheck %s
define void @broadcast() {
%zero = shufflevector <vscale x 16 x i8> undef, <vscale x 16 x i8> undef, <vscale x 16 x i32> zeroinitializer
ret void
}| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 876 | Can we assert that the Type is Fixed unless the shuffle is a broadcast? assert((ImprovedKind == TTI::SK_Broadcast || isa<FixedVectorType>(Tp)) && "Unexpected shuffle of scalable vector"); | |
Comment Actions
good point, I missed this because of that rv does not have such target api. I'll add it.
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 876 | yes，we can. | |
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 876 | hmm, we cannot assert here, SK_ExtractSubvector and such kind handle scalable vector as well. | |
Comment Actions
This looks like it would treat the default cost of a broadcast as scalarising the first n elements from a <vscale x n x iM> vector, which doesn't sound right. Unless we are assuming that the default vscale is 1.
I would suspect that the cost should either be invalid or to be "cheap" under assumption that the operation is always present. AArch64 has some cost overrides in its backend for certain types of scalable broadcasts.
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 876 | But we are casting to fixed vector type in the case statements below here - if scalable type is possible, then it will crash? Do you have an example of a scalable vector with any of the other shuffle kinds? | |
Comment Actions
Yes， This patch keeps same behavior as before (release mode).
I would suspect that the cost should either be invalid or to be "cheap" under assumption that the operation is always present. AArch64 has some cost overrides in its backend for certain types of scalable broadcasts.
without target override，we cannot compute scalable vector broadcasts here. So I think set invalid for scalabe vector here make sense.
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 87–88 | Not absolutely against this change, but I'm not a fan. This gives the impression this private helper function supports any type of vector, which is clearly not the case and hence why you return an invalid cost. Essentially most of the function implementations within this file specially cost the scalarisation path, which is just an invalid path for scalable vectors. For this reason I think it's better to catch these cases as early in the call chain as possible. This function taking a FixedVectorType means that you're more likely to realise you're doing something wrong at compile time rather than runtime. Although I'll concede this is a bit tenuous given it currently only has a single use, but then would we want to make similar changes to every *Overhead function when it's more likely the caller is just at fault. | |
| 878–880 | Based on the above personally I think getShuffleCost is wholly unsafe for scalable vector shuffles and I'd sooner see the "you probably didn't want to get here" code here. For example: case TTI::SK_Broadcast:
if (!isa<FixedVectorType>(Tp))
return InstructionCost::getInvalid();
return getBroadcastShuffleOverhead(cast<FixedVectorType>(Tp));This for me makes it clearer that if you'd rather not return an invalid cost then you'll need to fix the target specific implementation of this function. Presumably the other shuffle types below need the same treatment? | |
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 878–880 |
make sense to me.
Yes, we also need handle intrinsic like vector.reverse with scalable vector. | |
| llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
|---|---|---|
| 878–880 | nit: is it worth writing this as: if (auto *FVT = dyn_cast<FixedVectorType>(Tp)) return getBroadcastShuffleOverhead(FVT); return InstructionCost::getInvalid() | |
| 887 | (same suggestion here) | |
| llvm/test/Analysis/CostModel/RISCV/rvv-shuffle.ll | ||
| 5 | nit: -scalable-vectorization=off|on is an option to the LoopVectorizer, not the costmodel, so it won't have any effect when combined with -cost-model -analyze | |
| llvm/test/Analysis/CostModel/RISCV/rvv-shuffle.ll | ||
|---|---|---|
| 14 | I'm curious, why does this one have cost zero and all others are invalid? Maybe we're getting the min element count somewhere and assuming this is a scalar boxed in a fixed vector of length 1? | |
Not absolutely against this change, but I'm not a fan. This gives the impression this private helper function supports any type of vector, which is clearly not the case and hence why you return an invalid cost. Essentially most of the function implementations within this file specially cost the scalarisation path, which is just an invalid path for scalable vectors.
For this reason I think it's better to catch these cases as early in the call chain as possible. This function taking a FixedVectorType means that you're more likely to realise you're doing something wrong at compile time rather than runtime. Although I'll concede this is a bit tenuous given it currently only has a single use, but then would we want to make similar changes to every *Overhead function when it's more likely the caller is just at fault.