[PATCH] D103105: [AArch64] Optimise bitreverse lowering in ISel

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6868
+  EVT VT = Op.getValueType();
+
+  if (VT.isScalableVector() ||
----------------
Perhaps an assert here for `VT == MVT::v4i32 || VT == MVT::v2i64`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6869
+
+  if (VT.isScalableVector() ||
+      useSVEForFixedLengthVectorVT(VT, /*OverrideNEON=*/true)) {
----------------
How about these SVE checks, do we need tests for that?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6872
+    return LowerToPredicatedOp(Op, DAG, AArch64ISD::BITREVERSE_MERGE_PASSTHRU,
+                               true);
+  }
----------------
Nit: `/*OverrideNEON=*/true` ?

And we don't need the curly brackets for this if.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6879
+    REVB = DAG.getNode(AArch64ISD::REV32, DL, MVT::v16i8, Op.getOperand(0));
+  if (VT.getScalarSizeInBits() == 64)
+    REVB = DAG.getNode(AArch64ISD::REV64, DL, MVT::v16i8, Op.getOperand(0));
----------------
then this can be an `else`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6881
+    REVB = DAG.getNode(AArch64ISD::REV64, DL, MVT::v16i8, Op.getOperand(0));
+  SDValue Bitreverse = DAG.getNode(ISD::BITREVERSE, DL, MVT::v16i8, REVB);
+
----------------
Wanted add a nit about `Bitreverse` -> `BitReverse`, but perhaps we don't need it:

   return DAG.getNode(AArch64ISD::NVCAST, DL, VT,
                      DAG.getNode(ISD::BITREVERSE, DL, MVT::v16i8, REVB));


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103105