[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);
----------------
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));
```


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8453
+    default:
+      assert(0 && "Unexpected opcode");
+      return false;
----------------
craig.topper wrote:
> Can this be llvm_unreachable?
Yep


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8483
+  /// LHS of the TargetOpcode.
+  NodeExtensionHelper LHS;
+  /// RHS of the TargetOpcode.
----------------
craig.topper wrote:
> I think these can be references to NodeExtensionHelper? That would avoid needing to copy them. But I don't know if that would be fragile.
You're right, for this patch, they could be references.
But for the next patch they need to be copied because we apply the changes much later and the references can change between the "match" and "apply".

I've happy to introduce the copy only in the next patch (in particular since the next patch is probably more controversial/may not land.)

Let me know what you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134703