[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 Sep 12 03:18:17 PDT 2023
sdesmalen added a comment.
Just a few minor nits, otherwise looks good to me.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25775
+ SmallVector<SDValue, 8> TBLMask;
+ for (int Offset : ShuffleMask) {
+ // If we refer to the second operand then we have to add elements
----------------
nit: This should be called `Index` instead of `Offset`.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25783
+ // of the shufflevector, thus we are rejecting this transform.
+ if (Offset < 0 || (unsigned) Offset >= MaxOffset)
+ return SDValue();
----------------
If you pull out this condition to the top of the loop and return early for negative offsets, then in the rest of the loop you can assume Offset is `unsigned` and remove some of the `(int)` casts you have in there.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25788-25789
+
+ // It is still better to fill TBL mask to the actual hardware supported
+ // size with out of index elements.
+ for (unsigned i = 0; i < IndexLen - ElementsPerVectorReg; ++i)
----------------
nit: this statement is a bit unclear to me. Why is it better?
Do we want to say "Choosing an out-of-range index leads to the lane being zeroed".
Note that this is only relevant for i16+ types and for i8 types when the runtime VL < 2048.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25801
+ SDValue Shuffle;
+ if (IsSingleOp) {
+ Shuffle = DAG.getNode(ISD::INTRINSIC_WO_CHAIN, DL, ContainerVT,
----------------
nit: unnecessary curly braces.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25809
+ Op1, Op2, SVEMask);
+ }
+ Shuffle = convertFromScalableVector(DAG, VT, Shuffle);
----------------
Just to be safe, could you add an `else` branch here with `llvm_unreachable("Cannot lower shuffle without SVE2 TBL");` ?
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25959-25960
+ // Avoid producing TBL instruction if we don't know
+ // SVE register minimal size.
+ if (MinSVESize)
----------------
nit: odd line-break.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152205/new/
https://reviews.llvm.org/D152205
More information about the llvm-commits
mailing list