[PATCH] D134703: [RISCV][ISel] Refactor the formation of VW operations

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8327
+      // Some splats don't come with a VL.
+      if (OrigOperand.getNumOperands() > 2)
+        VL = OrigOperand.getOperand(2);
----------------
qcolombet wrote:
> craig.topper wrote:
> > I'm little confused by this check. The number of operands should be a property of the RISCVISD opcode. All RISCVISD::VMV_V_X_VL are supposed to have the same number of operands.
> I was surprised too but I've seen them in some tests.
> I assumed this was expected and didn't dig more.
> For instance, I see that we create some without `VL`s at line 1935 before this patch:
> ```
>   SDValue SplatZero = DAG.getNode(
>       RISCVISD::VMV_V_X_VL, DL, DstContainerVT, DAG.getUNDEF(DstContainerVT),
>       DAG.getConstant(0, DL, Subtarget.getXLenVT()));
> ```
> 
> You can find other instances here and there in that file. E.g., another one:
> ```
> // Create 2^eltbits - 1 copies of V2 by multiplying by the largest integer.
> SDValue Multiplier = DAG.getNode(RISCVISD::VMV_V_X_VL, DL, IntHalfVT,
>                                     DAG.getUNDEF(IntHalfVT),
>                                     DAG.getAllOnesConstant(DL, XLenVT));
> ```
@craig.topper ^^^
Tagging you to make sure you saw that I needed some feedback on that front.

Put differently, is it expected to have `VMV_V_X_VL` with no `VL` or should the snippet I showed need fixing?
(Unless I missed something and the `VL` are indeed added at some point.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134703