[PATCH] D145678: [RISCV] Model interleave and deinterleave shuffles in cost model

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Very close to LGTM, please apply changes and I think we're good to go.



================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:331
+    std::pair<InstructionCost, MVT> LT = getTypeLegalizationCost(Tp);
+    if (LT.second.isFixedLengthVector()) {
+      MVT EltTp = LT.second.getVectorElementType();
----------------
I think that LT.second must be fixed if the original type was.  Turn this into an assert?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:337
+        auto InterleaveMask = createInterleaveMask(Mask.size() / 2, 2);
+        auto DeinterleaveMask = createStrideMask(Mask[0], 2, Mask.size());
+        // Example sequence:
----------------
You need to check that Mask[0] is either 0 or 1 here.  (Or generalize the lowering.)


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:343
+        //   vwmaccu.vx	v10, a0, v9
+        if (equal(InterleaveMask, Mask))
+          return 2 * LT.first * getLMULCost(LT.second);
----------------
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.


================
Comment at: llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll:17
 ; CHECK-LABEL: 'interleave2_v8i32'
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 15 for instruction: %concat = shufflevector <4 x i32> %v0, <4 x i32> %v1, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 4 for instruction: %res = shufflevector <8 x i32> %concat, <8 x i32> poison, <8 x i32> <i32 0, i32 4, i32 1, i32 5, i32 2, i32 6, i32 3, i32 7>
----------------
This cost is also suspiciously high.  This really should be a single slideup.  Yet another follow on change.  :)


================
Comment at: llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll:43
+
+; TODO: getInstructionCost doesn't call getShuffleCost here because the shuffle changes length
+define {<4 x i8>, <4 x i8>} @deinterleave_2(<8 x i8> %v) {
----------------
luke wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145678/new/

https://reviews.llvm.org/D145678