[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:25686
+  unsigned BitsPerElt = VTOp1.getVectorElementType().getSizeInBits();
+  unsigned IndexLen = MinSVESize / BitsPerElt;
+  unsigned NumElts = VTOp1.getVectorNumElements();
----------------
Please rename this to something more descriptive, e.g. `ElementsPerVector`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25689
+  unsigned MaskSize = ShuffleMask.size();
+  unsigned MaxOffset = (1ULL << ((BitsPerElt >= 32) ? 32 : BitsPerElt)) - 1;
+  assert(NumElts <= IndexLen && MaskSize <= IndexLen &&
----------------
Seems more generic to write:

  uint64_t MaxOffset = APInt(BitsPerElt, -1, false).getZExtValue();


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25695
+  for (int Val : ShuffleMask) {
+    unsigned Offset = Val;
+    if (IsSingleOp) {
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25696
+    unsigned Offset = Val;
+    if (IsSingleOp) {
+      TBLMask.push_back(DAG.getConstant(Offset, DL, MVT::i64));
----------------
I don't think you need to distinguish `IsSingleOp` here, because if `IsSingleOp == true`, then `Offset > NumElts`, so the code below already covers it.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25706-25707
+      // number in hardware register minus number of elements in a type.
+      if (Offset > NumElts)
+        Offset += IndexLen - NumElts;
+      TBLMask.push_back(DAG.getConstant(Offset, DL, MVT::i64));
----------------
You'll need to update the real offset before checking whether Offset >= MaxOffset. For example, for vscale_range(16, 16) the index into the next vector would always be at least 256, which cannot be represented in an i8.

Moving this code above the previous condition fixes that.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25715
+  for (unsigned i = 0; i < IndexLen - NumElts; ++i)
+    TBLMask.push_back(DAG.getConstant(MaxOffset, DL, MVT::i64));
+
----------------
nit: you can simplify this to `-1`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25731-25732
+                    Op1, SVEMask));
+  } else {
+    if (Subtarget.hasSVE2()) {
+      Shuffle = convertFromScalableVector(
----------------
Combine this into an `else if`


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

https://reviews.llvm.org/D152205