[PATCH] D152205: [Aarch64][SVE]SVE2] Enable tbl, tbl2 for shuffle lowering for fixed vector types.
Sander de Smalen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 1 07:49:46 PDT 2023
sdesmalen added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25733
+ }
+ bool IsUndefOrZero = Op2.isUndef() || isZerosVector(Op2.getNode());
+ unsigned FillElements = IndexLen - NumElts;
----------------
I think you need to call this `Op2IsUndefOrZero`
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25745-25749
+ // Filling up the remaining positions of the mask with 255 because for one
+ // byte per element and maximum possible 2048-bits register size this is the
+ // last range value.
+ else if (IsUndefOrZero && Offset >= NumElts)
+ Offset = 255;
----------------
What should happen if the vector type is `<256 x i8>` and IsUndefOrZero is `false` ?
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25752-25753
+ }
+ // It is still better to fill TBL mask to the actual hardware supported
+ // size with out of index elements.
+ for (unsigned i = 0; i < FillElements; ++i)
----------------
I'm curious, why is that better?
Also, as my comment above suggests, for `<256 x i8>` vectors, `255` is just the last element of the first data-vector. There is also some test-coverage missing here.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25763
+ SDValue Shuffle;
+ if (IsUndefOrZero || Swap) {
+ Shuffle = convertFromScalableVector(
----------------
I think this condition can be removed? (if it has swapped, then the second vector must be zero or undef?)
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25768
+ DAG.getConstant(Intrinsic::aarch64_sve_tbl, DL, MVT::i32),
+ Op1, SVEMask));
+ } else {
----------------
What is the type of VecMask here? How can the type of VecMask here (single-vector TBL instruction) be the same as for the case of the SVE2 two-vector TBL instruction?
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25769-25770
+ Op1, SVEMask));
+ } else {
+ if (Subtarget->hasSVE2())
+ Shuffle = convertFromScalableVector(
----------------
nit:
} else if (Subtarget->hasSVE2()) {
...
} else
return SDValue();
?
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25152
+
+ bool Swap = false;
+ if (Op1.isUndef() || isZerosVector(Op1.getNode())) {
----------------
dtemirbulatov wrote:
> sdesmalen wrote:
> > It's not really clear to me what `Swap` and `IsUndefOrZero` are supposed to do. I would have expected it to look at the indices rather than the source vector operands. Can you add a comment explaining this logic?
> Ok, I will comment for those variables. Also, for example. If the mask is refers to only one operand then second operand would be already undefined at the time of lowering. I think we don't have to check the mask here.
There is an implicit assumption that a DAGCombine (or perhaps an InstCombine) has rewritten the second operand to be `undef`. It seems safer to look at the values in the Mask instead.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152205/new/
https://reviews.llvm.org/D152205
More information about the llvm-commits
mailing list