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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25854
+  if (llvm::all_of(ShuffleMask, [&NumElts](unsigned Val) {
+        return (Val >= NumElts && (Val < (NumElts * 2)));
+      })) {
----------------
sdesmalen wrote:
> Is the condition `&& (Val < NumElts * 2)` needed? This seems already covered by the `if (NumElts > IndexLen)` check above on line 25844.
yes, we need this check to understand that we just electing from operand 2 and operand 1 is unused.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25857
+    Op1IsUndefOrZero = true;
+    std::swap(Op1, Op2);
+  }
----------------
sdesmalen wrote:
> It's probably easier to update the ShuffleMask here. That simplifies the logic below as it avoids having to constantly distinguish between the `Op*IsUndefOrZero` cases.
hmm, ShuffleMask is readonly variable.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25861
+  // Currently maximum offset number that could be lowered is 255.
+  if ((!Op2IsUndefOrZero && !Op1IsUndefOrZero && IndexLen > 128) ||
+      IndexLen > 256)
----------------
sdesmalen wrote:
> Hardcoding a check for '128' doesn't seem right to me. It all depends on what can be represented in the element type of the instruction, i.e. for i8's the max value is 255. That means when targeting `vscale_range(16, 16)`, then 255 is a valid index into the first source vector.
yes and for second source operand the index starts from 256, so that is what we are checking here is we have to address to both operands then it is going to be 255 + index and we already established that both operands involved. 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25862
+  if ((!Op2IsUndefOrZero && !Op1IsUndefOrZero && IndexLen > 128) ||
+      IndexLen > 256)
+    return SDValue();
----------------
sdesmalen wrote:
> It doesn't seem right to look at the //number of indices// in the mask. I think rather it should be about the maximum index //value// in the mask?
We already checked all values in the mask.


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

https://reviews.llvm.org/D152205