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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov marked 4 inline comments as done.
dtemirbulatov added inline comments.


================
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)
----------------
sdesmalen wrote:
> 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.
I think that if we leave uninitialized remaining out of scope indexes then they will end up with 0 value and means extra work since 0 element almost always means 0 lane and with 255 value probably hardware could understand this is out of scope value, except <256 x i8> case which a it rarely than valid 0 index. The last available value for index here is 255 then it flips to 0.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25763
+  SDValue Shuffle;
+  if (IsUndefOrZero || Swap) {
+    Shuffle = convertFromScalableVector(
----------------
sdesmalen wrote:
> I think this condition can be removed? (if it has swapped, then the second vector must be zero or undef?)
No,  IsUndefOrZero tells us that only operand 0 is used. I will add a test and rename the variable.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25768
+                    DAG.getConstant(Intrinsic::aarch64_sve_tbl, DL, MVT::i32),
+                    Op1, SVEMask));
+  } else {
----------------
sdesmalen wrote:
> 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?
This is not correct, According to specification index size is similar for TBL, TBL2.


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

https://reviews.llvm.org/D152205