[PATCH] D72856: [ARM][MVE] Enable masked scatter

[Anna Welker via Phabricator via llvm-commits]

anwel marked 6 inline comments as done.
anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:542
+bool ARMTTIImpl::isLegalMaskedScatter(Type *Ty, MaybeAlign Alignment) {
+  if (!EnableMaskedGatherScatters || !ST->hasMVEIntegerOps())
+    return false;
----------------
dmgreen wrote:
> This can just call isLegalMaskedGather, like we do for isLegalMaskedStore
Fair point.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:292
     LLVM_DEBUG(dbgs() << "masked gathers: found an extending gather\n");
-    ResultTy = Extend->getType();
+    ResultTy = Root->getType();
     // The final size of the gather must be a full vector width
----------------
dmgreen wrote:
> This looks like it's gone back to an older version?
I think you are right. Thanks for catching that.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:459-465
+  if (Gathers.empty() && Scatters.empty())
     return false;
 
   for (IntrinsicInst *I : Gathers)
     lowerGather(I);
+  for (IntrinsicInst *I : Scatters)
+    lowerScatter(I);
----------------
dmgreen wrote:
> This might be better as 
>   bool Changed = false;
>   for (IntrinsicInst *I : Gathers)
>     Changed |= lowerGather(I);
>   for (IntrinsicInst *I : Scatters)
>     Changed |= lowerScatter(I);
> 
> As we might not be altering the gathers/scatters that we see, leaving them for the legalise pass to scalarise.
Good point. We do get the booleans from the functions' return value, so we should be using it.


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

https://reviews.llvm.org/D72856