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

Thanks for the latest round of changes. I have a few more comments, but this seems to be moving in the right direction.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25721
+  if (IsSingleOp) {
+    Shuffle = convertFromScalableVector(
+        DAG, VT,
----------------
nit: you can sink the `convertFromScalableVector` out of the if/else conditions, and moving it to the return.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25695
+  for (int Val : ShuffleMask) {
+    unsigned Offset = Val;
+    if (IsSingleOp) {
----------------
dtemirbulatov wrote:
> sdesmalen wrote:
> > What is the point of this variable?
> > 
> > In my previous comment I suggested that the calculation should be done on signed values because the ShuffleMask has signed values . Doing:
> > 
> >   for (int Val : ShuffleMask) {
> >     unsigned Offset = Val;
> >     // rest of code using Offset instead of Val
> >   }
> > 
> > does not solve that problem.
> hmm, not expecting anytging harmful here, according to the specification : "Non-negative elements in the mask represent an index."
> hmm, not expecting anything harmful here, according to the specification : "Non-negative elements in the mask represent an index.
That doesn't mean the value cannot be negative. I prefer the code to be explicit about what happens when the value is negative, rather than this being implicit by using an `unsigned` variable.

`ShuffleMask` is an array of signed integers. When you then iterate over this array of signed integers, it makes sense to make that variable signed as well.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-vector-shuffle.ll:963
+; CHECK-NEXT:    adrp x8, .LCPI43_0
+; CHECK-NEXT:    add x8, x8, :lo12:.LCPI43_0
+; CHECK-NEXT:    ld1d { z0.d }, p0/z, [x1]
----------------
Missing CHECK lines for this symbol.


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-vector-shuffle.ll:378
 }
+
+; CHECK: .LCPI23_0:
----------------
You have added these tests to `sve-streaming-mode-fixed-length-vector-shuffle.ll`, but the sve-streaming-mode-* tests should not contain any tests with specific `vscale_range(..)` attributes, because the point is more to test that no NEON is being used and streaming(-compatible) SVE is used instead.


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-vector-shuffle.ll:668
+
+define <16 x double> @shuffle_doublemask_size_unacceptable_form(ptr %a, ptr %b) #1 {
+; CHECK-LABEL: shuffle_doublemask_size_unacceptable_form:
----------------
What is unacceptable about the form?


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

https://reviews.llvm.org/D152205