[PATCH] D152205: [Aarch64][SVE]SVE2] Enable tbl, tbl2 for shuffle lowering for fixed vector types.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Just a few minor nits, otherwise looks good to me.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25775
+  SmallVector<SDValue, 8> TBLMask;
+  for (int Offset : ShuffleMask) {
+    // If we refer to the second operand then we have to add elements
----------------
nit: This should be called `Index` instead of `Offset`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25783
+    // of the shufflevector, thus we are rejecting this transform.
+    if (Offset < 0 || (unsigned) Offset >= MaxOffset)
+      return SDValue();
----------------
If you pull out this condition to the top of the loop and return early for negative offsets, then in the rest of the loop you can assume Offset is `unsigned` and remove some of the `(int)` casts you have in there.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25788-25789
+
+  // It is still better to fill TBL mask to the actual hardware supported
+  // size with out of index elements.
+  for (unsigned i = 0; i < IndexLen - ElementsPerVectorReg; ++i)
----------------
nit: this statement is a bit unclear to me. Why is it better?

Do we want to say "Choosing an out-of-range index leads to the lane being zeroed".
Note that this is only relevant for i16+ types and for i8 types when the runtime VL < 2048.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25801
+  SDValue Shuffle;
+  if (IsSingleOp) {
+    Shuffle = DAG.getNode(ISD::INTRINSIC_WO_CHAIN, DL, ContainerVT,
----------------
nit: unnecessary curly braces.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25809
+                    Op1, Op2, SVEMask);
+  }
+  Shuffle = convertFromScalableVector(DAG, VT, Shuffle);
----------------
Just to be safe, could you add an `else` branch here with `llvm_unreachable("Cannot lower shuffle without SVE2 TBL");` ?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25959-25960
 
+  // Avoid producing TBL instruction if we don't know
+  // SVE register minimal size.
+  if (MinSVESize)
----------------
nit: odd line-break.


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

https://reviews.llvm.org/D152205