[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 inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25733
+  }
+  bool IsUndefOrZero = Op2.isUndef() || isZerosVector(Op2.getNode());
+  unsigned FillElements = IndexLen - NumElts;
----------------
I think you need to call this `Op2IsUndefOrZero`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25745-25749
+    // Filling up the remaining positions of the mask with 255 because for one
+    // byte per element and maximum possible 2048-bits register size this is the
+    // last range value.
+    else if (IsUndefOrZero && Offset >= NumElts)
+      Offset = 255;
----------------
What should happen if the vector type is `<256 x i8>` and IsUndefOrZero is `false` ?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25752-25753
+  }
+  // It is still better to fill TBL mask to the actual hardware supported
+  // size with out of index elements.
+  for (unsigned i = 0; i < FillElements; ++i)
----------------
I'm curious, why is that better?

Also, as my comment above suggests, for `<256 x i8>` vectors, `255` is just the last element of the first data-vector. There is also some test-coverage missing here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25763
+  SDValue Shuffle;
+  if (IsUndefOrZero || Swap) {
+    Shuffle = convertFromScalableVector(
----------------
I think this condition can be removed? (if it has swapped, then the second vector must be zero or undef?)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25768
+                    DAG.getConstant(Intrinsic::aarch64_sve_tbl, DL, MVT::i32),
+                    Op1, SVEMask));
+  } else {
----------------
What is the type of VecMask here? How can the type of VecMask here (single-vector TBL instruction) be the same as for the case of the SVE2 two-vector TBL instruction?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25769-25770
+                    Op1, SVEMask));
+  } else {
+    if (Subtarget->hasSVE2())
+      Shuffle = convertFromScalableVector(
----------------
nit:

  } else if (Subtarget->hasSVE2()) {
     ...
  } else
    return SDValue();

?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25152
+
+  bool Swap = false;
+  if (Op1.isUndef() || isZerosVector(Op1.getNode())) {
----------------
dtemirbulatov wrote:
> sdesmalen wrote:
> > It's not really clear to me what `Swap` and `IsUndefOrZero` are supposed to do. I would have expected it to look at the indices rather than the source vector operands. Can you add a comment explaining this logic?
> Ok, I will comment for those variables. Also, for example.  If the mask is refers to only one operand then second operand would be already undefined at the time of lowering. I think we don't have to check the mask here.
There is an implicit assumption that a DAGCombine (or perhaps an InstCombine) has rewritten the second operand to be `undef`. It seems safer to look at the values in the Mask instead.


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

https://reviews.llvm.org/D152205