[PATCH] D93312: [RISCV] Add ISel support for RVV .vx and .vi forms

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:347
+
+    for (auto VT : MVT::integer_scalable_vector_valuetypes())
+      setOperationAction(ISD::SPLAT_VECTOR, VT, Legal);
----------------
craig.topper wrote:
> craig.topper wrote:
> > I'm not sure this should be all scalable types. Typically we only mark the legal types.
> On second thought, I guess this is ok since we're marking it Legal. Its no different than an operation that defaults to legal. But we are probably going to have to do something different for i1 vectors.
Yeah, I was a bit iffy about it too. I think it's okay for now in that it's currently no worse than `BUILD_VECTOR` being assumed legal and being used otherwise. But I could also create a list of supported vector `MVT`s and be specific about which are allowed. We are likely going to need that sort of list in the future anyway.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:417
+                              vti.Vector, vti.Vector, XLenVT, vti.Mask, vti.SEW,
+                              vti.LMul, vti.RegClass, vti.RegClass, ImmType>;
+  }
----------------
craig.topper wrote:
> This might not catch all immediates it could. The operand of the splat_vector is going to be XLenVT. When the scalar element type is less than XLenVT, only some of the bits of the splat_vector operand are going to be used so the other bits should be ignored for the purposes of checking if the immediate is simm5. We might get lucky because DAGTypeLegalizer::PromoteIntRes_Constant prefers sign_extend.
That's a good point. I do think `PromoteIntRes_Constant` is saving us here. Is this something that we should fix without a test case, or something to note for later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93312