[PATCH] D157976: [RISCV] Use materialization cost when lowering constant build_vector

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Please separate the removal of the immediate restriction on the VID into it's own change, either before or after.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3294
+    if (isPowerOf2_32(StepDenominator) && (SplatStepVal >= 0 || StepDenominator == 1)) {
+      AddLowering(Cost, [=, &DAG, &Subtarget]() {
+        MVT VIDVT =
----------------
Copying everything by value here to avoid scope issues is rather subtle.  I don't spot a scope bug here, but I'm not a huge fan of the code structure. 


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3361
 
-    SDValue Vec = DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, ViaVecVT,
-                              DAG.getUNDEF(ViaVecVT),
-                              DAG.getConstant(SplatValue, DL, XLenVT),
-                              DAG.getConstant(0, DL, XLenVT));
-    if (ViaVecLen != 1)
-      Vec = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL,
-                        MVT::getVectorVT(ViaIntVT, 1), Vec,
-                        DAG.getConstant(0, DL, XLenVT));
-    return DAG.getBitcast(VT, Vec);
+    // Base cost of 1 for vmv.s.x.
+    unsigned Cost = 1;
----------------
This is off when we can use a vmv.v.i to perform the insert.  


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-fp-shuffles.ll:195
 ; RV64-NEXT:    addi a0, a0, %lo(.LCPI8_0)
 ; RV64-NEXT:    vlse64.v v10, (a0), zero
 ; RV64-NEXT:    vsetivli zero, 1, e8, mf8, ta, ma
----------------
Not this change, but there's something odd here.  Unless I'm misreading this, we should be doing a splat of 2.0 as double here.  That's the hex value 0x4000000000000000 which is a constant we can materialize in two instructions at worst.  (1 with zbs).  

It looks maybe we've got a problem with how we lower constant splats of floating point types?  We should be able to use the integer mat path, and it looks like we're not doing so?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157976