[PATCH] D89576: [SVE][CodeGen] Lower scalable masked scatters

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

This patch got bigger incrementally and is now now really substantial and a bit difficult to review. Are there parts you can maybe post as a separate patch?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9387
+  SDValue SplatValue = LHS.getOperand(0);
+  if (SplatValue.getSimpleValueType() != MVT::i64)
+    return false;
----------------
This seems specific to AArch64 which has 64bit pointers. But I'd think this check is not needed to begin with, because the MaskedGS BasePtr must be a scalar pointer, or a vector of pointers, so the splat value would always be a pointer type (which I assume this code is checking).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16152
+                                             EVT MemVT, SDValue Offsets) const {
+  bool ScaledIndex = (IndexType == ISD::SIGNED_SCALED) ||
+                     (IndexType == ISD::UNSIGNED_SCALED);
----------------
nit: s/ScaledIndex/IsScaledIndex/
nit: s/SignedIndex/IsSignedIndex/


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3688
+bool AArch64TargetLowering::isLegalMaskedGSIndexType(EVT VT) const {
+ if (VT == MVT::nxv2i32 || VT == MVT::nxv4i32 || VT == MVT::nxv2i64)
+   return true;
----------------
kmclaughlin wrote:
> sdesmalen wrote:
> > Is that right though? nxv2i32 is not a legal type.
> > 
> > I feel like the interface is actually asking the wrong question. I think the question the DAGCombiner wants to ask is: "should I remove the sign/zero-extend for the gather/scatter index". For SVE, that is true for indices that are extended like:
> >   nxv4i32 -> nxv4i64
> >   nxv8i32 -> nxv8i64
> >   ..
> > because the instruction can do this. It adds little value to add this for:
> >   nxv2i32
> > because the type itself needs to be promoted to nxv2i64 anyway.
> > 
> > If it asks the right question and you make it work for nxv8i32, then codegen for the following test would much improve:
> > ```define void @masked_scatter_nxv8f32_zext(<vscale x 8 x float> %data, float* %base, <vscale x 8 x i32> %indexes, <vscale x 8 x i1> %masks) nounwind #0 {
> >   %ext = zext <vscale x 8 x i32> %indexes to <vscale x 8 x i64>
> >   %ptrs = getelementptr float, float* %base, <vscale x 8 x i64> %ext
> >   call void @llvm.masked.scatter.nxv8f32(<vscale x 8 x float> %data, <vscale x 8 x float*> %ptrs, i32 0, <vscale x 8 x i1> %masks)
> >   ret void
> > }
> > declare void @llvm.masked.scatter.nxv8f32(<vscale x 8 x float>, <vscale x 8 x float*>, i32, <vscale x 8 x i1>)
> > ```
> > (for which tests are currently missing in the patch) 
> Changed `isLegalMaskedGSIndexType` to instead return true only if the index is extended from i32 & the number of elements is >=4. The test above has been added to `sve-masked-scatter-legalise.ll`, along with some other tests where the offset vector type is illegal.
After this change, I think `isLegalMaskedGSIndexType` is a misnomer, because `nxv8i32` is not a legal type for SVE.
How about `bool shouldRemoveExtendFromGSIndex` ? (defaulting to `false`)


================
Comment at: llvm/test/CodeGen/AArch64/sve-masked-scatter-legalise.ll:1
+; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve < %s | FileCheck %s
+
----------------
nit: please use American spelling for the file name, i.e. legalize :)


================
Comment at: llvm/test/CodeGen/AArch64/sve-masked-scatter-legalise.ll:39
+; Code generate the worst case scenario when all vector types are illegal.
+define void @masked_scatter_nxv32i32(<vscale x 32 x i32> %data, i32* %base, <vscale x 32 x i32> %offsets, <vscale x 32 x i1> %mask) {
+; CHECK-LABEL: masked_scatter_nxv32i32:
----------------
nit: If you pass in %data as a `<vscale x 32 x i8>` and sign-extend it to `<vscale x 32 x i32>`, then your patch won't need to depend on D90219.


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

https://reviews.llvm.org/D89576