[PATCH] D123653: [NFC][RISCV][CodeGen] Use ArrayRef in TargetLowering functions

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In general I'm in favour of this sort of thing but only if they're //logically// grouped and we're not just trying to save lines of code. We should only be using these overloads when they increase comprehension: I stopped reviewing at a certain point as I felt I was becoming repetitive.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:443
+      setOperationAction(
+          {ISD::INSERT_VECTOR_ELT, ISD::EXTRACT_VECTOR_ELT, ISD::VECREDUCE_ADD,
+           ISD::VECREDUCE_AND, ISD::VECREDUCE_OR, ISD::VECREDUCE_XOR,
----------------
Maybe keep insert/extract separate? They're conceptually different enough to reductions to warrant their own actions.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:455
       // Mask VTs are custom-expanded into a series of standard nodes
-      setOperationAction(ISD::TRUNCATE, VT, Custom);
-      setOperationAction(ISD::CONCAT_VECTORS, VT, Custom);
-      setOperationAction(ISD::INSERT_SUBVECTOR, VT, Custom);
-      setOperationAction(ISD::EXTRACT_SUBVECTOR, VT, Custom);
-
-      setOperationAction(ISD::INSERT_VECTOR_ELT, VT, Custom);
-      setOperationAction(ISD::EXTRACT_VECTOR_ELT, VT, Custom);
+      setOperationAction({ISD::TRUNCATE, ISD::CONCAT_VECTORS,
+                          ISD::INSERT_SUBVECTOR, ISD::EXTRACT_SUBVECTOR,
----------------
Again I'm not sure whether these all belong together and we're not saving much code by grouping them together.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:465
 
-      setOperationAction(ISD::VECREDUCE_AND, VT, Custom);
-      setOperationAction(ISD::VECREDUCE_OR, VT, Custom);
-      setOperationAction(ISD::VECREDUCE_XOR, VT, Custom);
-
-      setOperationAction(ISD::VP_REDUCE_AND, VT, Custom);
-      setOperationAction(ISD::VP_REDUCE_OR, VT, Custom);
-      setOperationAction(ISD::VP_REDUCE_XOR, VT, Custom);
+      setOperationAction({ISD::VP_AND, ISD::VP_OR, ISD::VP_XOR,
+                          ISD::VECREDUCE_AND, ISD::VECREDUCE_OR,
----------------
VP binary ops and reductions don't belong together, if you ask me.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:546
 
-      setOperationAction(ISD::LOAD, VT, Custom);
-      setOperationAction(ISD::STORE, VT, Custom);
-
-      setOperationAction(ISD::MLOAD, VT, Custom);
-      setOperationAction(ISD::MSTORE, VT, Custom);
-      setOperationAction(ISD::MGATHER, VT, Custom);
-      setOperationAction(ISD::MSCATTER, VT, Custom);
-
-      setOperationAction(ISD::VP_LOAD, VT, Custom);
-      setOperationAction(ISD::VP_STORE, VT, Custom);
-      setOperationAction(ISD::VP_GATHER, VT, Custom);
-      setOperationAction(ISD::VP_SCATTER, VT, Custom);
+      setOperationAction({ISD::LOAD, ISD::STORE, ISD::MLOAD, ISD::MSTORE,
+                          ISD::MGATHER, ISD::MSCATTER, ISD::VP_LOAD,
----------------
This is another big jumble of operations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123653