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


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8434
+  bool areVLAndMaskCompatible(const SDNode *Root) const {
+    std::pair<SDValue, SDValue> MaskAndVL = getMaskAndVL(Root);
+    return isMaskCompatible(MaskAndVL.first) &&
----------------
This can also use structured binding


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


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8483
+  /// LHS of the TargetOpcode.
+  NodeExtensionHelper LHS;
+  /// RHS of the TargetOpcode.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134703