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

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:738
 
+InstructionCost RISCVTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val,
+                                                 unsigned Index) {
----------------
reames wrote:
> 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.  
Thanks, I missed some special cases of insertelement, already added.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:745
+
+  // This type is legalized to a scalar type.
+  if (!LT.second.isVector())
----------------
reames wrote:
> 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.
Thanks for pointing this. If a scalable vector type is not supported in the target, `getTypeLegalizationCost` would return {InvalidCost, itself}. I missed this case. Now I add an early exit to make sure returning InvalidCost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133007