Interleave and deinterleave shuffles are lowered by a more efficient
sequence if the element size is smaller than ELEN.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll | ||
---|---|---|
43 | The motivation behind this patch is to use this shuffle cost for the cost of an interleaved memory access, so the shuffles in this test case are the same as the ones generated by the loop vectorizer. Unfortunately, because these shuffles change the length of the vector, TargetTransformImpl.h returns -1 in getInstructionCost: case Instruction::ShuffleVector: { auto *Shuffle = dyn_cast<ShuffleVectorInst>(U); // ... if (Shuffle->changesLength()) { // ... return CostKind == TTI::TCK_RecipThroughput ? -1 : 1; } Which makes it difficult to test. Any ideas here? |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
340 | I excluded the li here since in loops it's an invariant that's hoisted out. Do we still need to be pessimistic and include it here? |
Very close to LGTM, please apply changes and I think we're good to go.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
331 | I think that LT.second must be fixed if the original type was. Turn this into an assert? | |
337 | You need to check that Mask[0] is either 0 or 1 here. (Or generalize the lowering.) | |
343 | Creating the masks just to check them this way is a bit inefficient. I'm fine with this landing as is, but we probably need to generalize out isInterleaveMask utilities. We already have that code in the lowering, so this is at least our second copy. Follow up change is fine. | |
llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll | ||
17 | This cost is also suspiciously high. This really should be a single slideup. Yet another follow on change. :) | |
43 | Seems like we need to make a change to generic code and TTI interface. There's no reason we shouldn't be costing length changing shuffles. A generic means would be to model a non-length changing shuffle followed by a vector insert or extract. This is definite a separate change. |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
331 | I thought so too, but <2 x i64> can get scalarized to i64 on zve32x! The zve32x test case in https://reviews.llvm.org/D145697 exercises this | |
343 | I agree, I was hoping there would be something equivalent to isReverse()/isTransposeMask() in ShuffleVectorInst. |
llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll | ||
---|---|---|
17 | If you stick this into llc then the shuffle actually disappears entirely. |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
337 | This remains open. This is the only blocking item. |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
337 | Sorry for the delay, was wrestling with arc |
I think that LT.second must be fixed if the original type was. Turn this into an assert?