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

Sjoerd Meijer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 7 03:51:04 PST 2020


SjoerdMeijer added a comment.

looks good to me too, other than the irrelevant nits, I have one question though, see inlined.



================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:524
+
+  // This method is called to 2 places
+  //  - once from the vectorizer with a scalar type, in which case we need to
----------------
nit: should it be called "at" or "in" 2 places?


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:529
+  //  - and also from the masked intrinsic lowering pass with the actual vector
+  //  type. For MVE, we want to custom legalise any gathers that we can to mve
+  //  intrinsics, and expand all the rest. So if we are here, we know we want
----------------
dmgreen wrote:
> Maybe expand this a little to "For MVE, we have a custom lowering pass that will already have custom legalised any gathers that we can to mve intrinsics, and want to expand all the rest. The pass runs before the masked intrinsic lowering pass, so if we are here, we know we want  to expand."
nit: mve -> MVE


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:86
+static bool isLegalAlignment(unsigned NumElements, unsigned ElemSize,
+                      unsigned Alignment) {
+  // Do only allow non-extending v4i32 gathers for now
----------------
nit: indentation


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:96
+  // @llvm.masked.gather.*(Ptrs, alignment, Mask, Src0)
+  // Attempt to turn the masked gather in I into a mve intrinsic
+  // Potentially optimising the addressing modes as we do so.
----------------
nit: mve -> MVE


================
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;
----------------
nit: are we checking types too? Just omit "or vector type"?


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:117
+  Value *Load = nullptr;
+  // Look through bitcast instruction if #elements is same
+  if (auto *BitCast = dyn_cast<BitCastInst>(Ptr)) {
----------------
is same -> is the same


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


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:153
+  }
+  LLVM_DEBUG(dbgs() << "masked gathers: could not build masked gather\n");
+  return false;
----------------
and this is never reached? Which makes sense probably, because we should be able to lower it?


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

https://reviews.llvm.org/D71743





More information about the llvm-commits mailing list