[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:25692-25705
+  switch (BitsPerElt) {
+  default:
+    llvm_unreachable("unexpected element type for vector");
+  case 8:
+    MaxOffset = std::numeric_limits<uint8_t>::max();
+    break;
+  case 16:
----------------
If you define `MaxOffset` as `(1 << BitsPerElt) - 1`, you can remove the switch statement?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25707-25712
+  bool IsSingleOp = ShuffleVectorInst::isSingleSourceMask(ShuffleMask);
+
+  // Ignore two operands case if not SVE2 or all indexes numbers
+  // could be represented.
+  if (!IsSingleOp && (!Subtarget.hasSVE2() || MinSVESize != MaxSVESize))
+    return SDValue();
----------------
Please move this code up after

  if (!MinSVESize)
    return SDValue();


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25716
+  if (IsSingleOp) {
+    for (unsigned Val : ShuffleMask)
+      TBLMask.push_back(DAG.getConstant(Val, DL, MVT::i64));
----------------
You're using `unsigned` here, but ShuffleMask is actually a vector of `int`s. `undef` elements are represented as index `(int) -1`. When you're comparing this with MaxOffset for example, things get a little confusing and I think this is hiding some bugs.

Can you instead make all the ShuffleMask values use signed integers?


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

https://reviews.llvm.org/D152205