[PATCH] D125856: [RISCV] Add cost model for SK_Reverse

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:191
+    static const CostTblEntry ShuffleTbl[] = {
+        {TTI::SK_Reverse, MVT::nxv1i64, 1},
+        {TTI::SK_Reverse, MVT::nxv2i64, 1},
----------------
Why do we need a table to return a 1 for every legal scalable vector type? There's gotta be better ways to do that.

I don't believe that 1 is the correct answer for RISC-V though. It's a vid.v, a vrsub.vx, and a vrgather.vv if I remember correctly. That's at least a cost of 3 assuming an optimal implementation of vrgather.vv.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:272
 
+InstructionCost RISCVTTIImpl::getMemoryOpCost(unsigned Opcode, Type *Ty,
+                                              MaybeAlign Alignment,
----------------
This doesn't seem to go with the title of this patch.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:288
+  // The code-generator is currently not able to handle scalable vectors
+  // of <vscale x 1 x eltty> yet, so return an invalid cost to avoid selecting
+  // it. This change will be removed when code-generation for these types is
----------------
There are no problems with <vscale x 1 x eltty> in the code generator that I'm aware of. Please explain.


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/riscv-vector-reverse.ll:3
+; REQUIRES: asserts
+; RUN: opt -loop-vectorize -dce -instcombine -mtriple riscv64-linux-gnu \
+; RUN:   -mattr=+v -debug-only=loop-vectorize \
----------------
Shouldn't a reverse test have reverse intrinsics in it? Or did instcombine remove them?


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/riscv-vector-reverse.ll:4
+; RUN: opt -loop-vectorize -dce -instcombine -mtriple riscv64-linux-gnu \
+; RUN:   -mattr=+v -debug-only=loop-vectorize \
+; RUN:   -riscv-v-vector-bits-min=128 -S < %s 2>&1 | FileCheck %s
----------------
Do any of the check lines in this file come from -debug-only=loop-vectorize?


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/riscv-vector-reverse.ll:7
+
+define dso_local void @vector_reverse_i64(ptr nocapture noundef writeonly %A, ptr nocapture noundef readonly %B, i32 noundef signext %n) local_unnamed_addr #0 {
+; CHECK-LABEL: @vector_reverse_i64(
----------------
Remove unneeded attributes please


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125856