[PATCH] D132458: [LoopVectorize] Synthesize mask operands for vector variants as needed

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.
Herald added a subscriber: StephenFan.

Hi @huntergr, I've only partially reviewed this I'm afraid, but here are the comments I have so far. I'll try to review more this week!



================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:129
+
+  unsigned getParamIndexForMask() const {
+    auto MaskPos = getParamIndexForOptionalMask();
----------------
Might be worth adding `///` comments here, since the others all have them?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:134
+
+    llvm_unreachable("Requested paramater index of non-existent mask!");
+  }
----------------
I think this will be compiled away in a release build, right? So really it's just a non-release wrapper around `getParamIndexForOptionalMask`. Given it's only called in one place is it worth just making `getParamIndexForOptionalMask` public instead and putting an assert in LoopVectorize.cpp that a mask exists?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:137
+
+  bool isMasked() const { return getParamIndexForOptionalMask().has_value(); }
+
----------------
This function is never called - can it be deleted?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3474
+  // function that does use a mask and synthesize an all-true mask.
+  if (!VecFunc && TTI.emitGetActiveLaneMask() != PredicationStyle::None) {
+    Shape = VFShape::get(*CI, VF, /*HasGlobalPred=*/true);
----------------
This looks a little strange to me. In my mind, the ability to emit an active lane mask based on two integer inputs is orthogonal to how cheap it is to broadcast a true bit across a predicate. For example, an architecture may cheaply support the latter, but not the former. Maybe X86 is such an example? Can we not just let the mask cost decide the behaviour? That way you can simplify this to just

  if (!VecFunc) {
     ...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3502
+    if (Variant)
+      *Variant = VecFunc;
     NeedToScalarize = false;
----------------
Do we really need both the `Variant` and the `NeedToScalarize` parameter? It looks naively that setting `Variant = VecFunc` is synonymous with `NeedToScalarize = false`. I haven't looked into this in detail so I could be wrong, but it might make more sense to remove the `NeedToScalarize` in favour of setting `Variant`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8380
         // version of the instruction.
+        if (Variant)
+          return false;
----------------
Doesn't this mean we may end up picking the least optimal VF? For example, if there are v2i32 and v4i32 masked variants we'll only ever pick the v2i32, i.e. the lowest VF?


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

https://reviews.llvm.org/D132458