[PATCH] D94883: [CodeGen][SelectionDAG]Add new intrinsic experimental.vector.reverse

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10817
+  SDValue V = getValue(I.getOperand(0));
+  assert(VT == V.getValueType() && "Malformed experimental.vector.reverse!");
+
----------------
I think we can drop the "experimental" here now?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10819
+
+  // VECTOR_SHUFFLE doesn't support a scalable mask so use a dedicated node.
+  if (VT.isScalableVector()) {
----------------
Perhaps worth explaining that we're using VECTOR_SHUFFLE to implement the VECTOR_REVERSE operation for fixed length vectors? It confused me a bit at first trying to understand why we jump suddenly into VECTOR_SHUFFLE that's all.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10825
+
+  // Use VECTOR_SHUFFLE to maintain original behaviour for fixed-length vectors.
+  SmallVector<int, 8> Mask;
----------------
VECTOR_REVERSE?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:293
+  case ISD::VECTOR_REVERSE:
+    return "experimental_vector_reverse";
   case ISD::CARRY_FALSE:                return "carry_false";
----------------
I think this can just be "vector_reverse" because it's a generic opcode and not experimental?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5888
 
+SDValue AArch64TargetLowering::LowerVECTOR_REVERSE(SDValue Op,
+                                                   SelectionDAG &DAG) const {
----------------
Just for reference, if all the lowering operation does is create the ISD_VECTOR_REVERSE with the same arguments then you can actually just mark the operation as Legal and avoid creating the LowerVECTOR_REVERSE function as it should happen automatically. If we have plans to use SVE for fixed length vectors in future then it might be useful to keep this custom function anyway.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:253
+
 def SDT_AArch64Rev   : SDTypeProfile<1, 1, [SDTCisVec<0>, SDTCisSameAs<0,1>]>;
+def AArch64rev       : SDNode<"ISD::VECTOR_REVERSE", SDT_AArch64Rev>;
----------------
craig.topper wrote:
> Why not put this in TargetSelectionDAG.td?
Yeah, I think you need to add something to llvm/include/llvm/Target/TargetSelectionDAG.td along the lines of

def vector_reverse : SDNode<"ISD::VECTOR_REVERSE",
    SDTypeProfile<1, 1, [SDTCisVec<0>, SDTCisSameAs<0,1>]>,[]>;

because you've added a generic ISD opcode in this patch. In places where you've got patterns using "AArch64rev" you can just then use "vector_reverse" instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94883