[PATCH] D133007: [RISCV] Add cost model for vector insert/extract element.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Generally quite reasonable, a couple comments to address before a likely LGTM.



================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:738
 
+InstructionCost RISCVTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val,
+                                                 unsigned Index) {
----------------
This routine is used for both insertelement and extractelement.  Your comments apply only to extract.  I think you're either a) missing a opcode early return, or b) need to give corresponding insert comments.  


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:745
+
+  // This type is legalized to a scalar type.
+  if (!LT.second.isVector())
----------------
This line is interesting as it implies we should probably have a test check configuration for cases where the vector type is scalarized.  Can you add a run line (precommit it please) which *doesn't* have float, but does have vector?  Just to make sure we have this case exercised.

Hm, actually, thinking about this.  There may be an interesting case here.  What happens if we're costing a scalable vector which can't be legalized via scalarizing?  Digging through the code, it looks like we return LT.first as invalid in that case.  I think we need to propagate that here to be safe.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:749
+
+  if (Index != -1U) {
+    // The type may be split. For fixed-width vectors we can normalize the
----------------
Ok, the following is an *optional* suggestion.  Feel free to include it or ignore.

This might be more readable if structured as:
InstructionCost SlideDownCost = Index == 0 ? 0: 1;
InstructionCost MatCost = (your two special cases)
return 1 + SlideDownCost + MatCost.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133007