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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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:
> 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.)
Those are both bugs. I guess they don't get noticed because isel matches them into the .vx or .vi form of the instruction that uses them. I'll fix them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134703