[PATCH] D71743: [ARM][MVE] Enable masked gathers from vector of pointers

[Anna Welker via Phabricator via llvm-commits]

anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:86
+
+bool isLegalAlignment(unsigned NumElements, unsigned ElemSize,
+                      unsigned Alignment) {
----------------
dmgreen wrote:
> anwel wrote:
> > dmgreen wrote:
> > > Any function that is only used in this file can be static.
> > Do you mean private?
> This is global scope I think? So the function can be static. Same for LowerGather below.
Ah, ok.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:108
+    LLVM_DEBUG(dbgs() << "masked gathers: instruction does not have valid "
+                      << "alignment or vector type \n");
+    return false;
----------------
SjoerdMeijer wrote:
> nit: are we checking types too? Just omit "or vector type"?
Yes, we are actually checking both, so I changed the function name to reflect that.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:147
+
+  if (Load) {
+    LLVM_DEBUG(dbgs() << "masked gathers: successfully built masked gather\n");
----------------
SjoerdMeijer wrote:
> I might be wrong, but it looks like `Load` is always set here? Do we need to check it here?
You are right, the checks for `Load` are superfluous


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

https://reviews.llvm.org/D71743