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

[Dinar Temirbulatov via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25847
+  if (NumElts > IndexLen || (MaskSize != NumElts && (2 * NumElts > IndexLen)) ||
+      isZerosVector(Op1.getNode()) || isZerosVector(Op2.getNode()))
+    return SDValue();
----------------
sdesmalen wrote:
> There are two things not right about this:
> 1. The **contents** of `Op1` and `Op2` shouldn't matter for the lowering of the shuffle, so there is no reason to bail out here. (If you remove this, does any test fail?)
> 2. `isZerosVector` will never return true, because `Op1` and `Op2` are `INSERT_SUBVECTOR` nodes because they are assigned with `convertToScalableVector()`, and `INSERT_SUBVECTOR` is not considered in `isZerosVector`.
Sorry, I left isZerosVector() check by accident.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25859
+    Op1IsUnused = true;
+    std::swap(Op1, Op2);
+  }
----------------
sdesmalen wrote:
> What is the meaning of 'Op1IsUnused' after swapping Op1 and Op2? Should that be Op2IsUnused now?
> 
> To me it seems better to update the ShuffleMask directly and remove the need for `Op*IsUnused` altogether
replaced both variables with a single one 'IsSingleOp'.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25869
+      (Subtarget->hasSVE2() && IndexLen < (MaxOffset / 2) &&
+       MinSVESize == MaxSVESize)) {
+
----------------
dtemirbulatov wrote:
> sdesmalen wrote:
> > Please check this condition earlier up for an early bail out! (at same place where it checks MinSVESize)
> No, Most of those checks related to support TBL2 both opperand used and at the place where MinSVESize check we don't know content of the mask, thus could not tell that both operand would be used or not.
And for a single operand case it is enough to know just MinSVESize.


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

https://reviews.llvm.org/D152205