[PATCH] D153948: [RISCV][NFC] Refactor lowerToScalableOp.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In general I think this is a good idea, thanks. Whether or not something has a merge or mask is intrinsic to the op itself, not really a codegen choice. So I think this patch makes that more explicit, rather than relying on us specifying the right parameters. Plus it reduces copy/paste so that's a win.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4567
+  default:
+    llvm_unreachable("invalid opcode");
+    // clang-format off
----------------
This error message could probably be a little more explicit


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4625
+bool RISCVTargetLowering::hasMergeOp(unsigned Opcode) const {
+  assert(Opcode > RISCVISD::FIRST_NUMBER &&
+         Opcode <= RISCVISD::STRICT_VFROUND_NOEXCEPT_VL && "not a valid op");
----------------
I wonder if we want stronger checks here so people don't insert an op in the wrong place and have weird stuff happen.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:908
+  /// Get a RISCV target specified VL op for a given SDNode.
+  unsigned getRISCVVLOp(SDValue Op) const;
+  /// Return true if a RISCV target specified op does have a merge operand.
----------------
I think these can all be `static`? Or, is there any need for them to be member functions at all?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:909
+  unsigned getRISCVVLOp(SDValue Op) const;
+  /// Return true if a RISCV target specified op does have a merge operand.
+  bool hasMergeOp(unsigned Opcode) const;
----------------
`does have` -> `has`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153948