[PATCH] D53420: [IAI, LV] Avoid creating a scalar epilogue due to gaps in interleave-groups when optimizing for size

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

LGTM, only minor comments and optional suggestions.



================
Comment at: include/llvm/Analysis/VectorUtils.h:312
+  /// Return true if this Group requires that the last VF iterations will be
+  /// peeled and executed sequentially due to gaps.
+  bool requiresScalarEpilogue() const {
----------------
Fix comment - this Group may require that at-least a single scalar iteration will run after the vector loop, which potentially requires VF(*UF) such scalar iterations when the trip count is a multiple of VF(*UF).


================
Comment at: include/llvm/Analysis/VectorUtils.h:317
+    if (getNumMembers() == getFactor() ||
+        getMember(getFactor() - 1))
+      return false;
----------------
clang-format?


================
Comment at: lib/Analysis/VectorUtils.cpp:940
+        << "LV: Invalidate candidate interleaved group due to gaps that "
+           "require a scalar epilogue.\n");
+    releaseGroup(Ptr);
----------------
May also indicate which group is being invalidated, e.g., dump along its insert pos insn.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1140
   bool requiresScalarEpilogue() const {
-    return InterleaveInfo.requiresScalarEpilogue();
+    return InterleaveInfo.requiresScalarEpilogue() && EpilogCreationAllowed;
   }
----------------
Is this change really needed? If a scalar epilogue is not allowed, all interleave groups that require it are invalidated, and `InterleaveInfo.requiresScalarEpilogue()` should return false. Right?

If needed, slight preference to place the method call second.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1153
 
+  bool EpilogCreationAllowed = true;
+
----------------
Document this new field.

Technically speaking, the scalar epilogue might need to be destroyed rather than being "created", as it's based on the original *existing* version . Perhaps a more accurate name is `IsScalarEpilogueAllowed`, or `CanEnsureScalarEpilogue`.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4612
+  // Invalidate interleave groups that require an epilogue.
+  if (InterleaveInfo.requiresScalarEpilogue())
+    InterleaveInfo.invalidateGroupsRequiringScalarEpilogue();
----------------
Suffice to check this only once, preferably in callee only, which also checks the EpilogCreationAllowed flag.


================
Comment at: test/Transforms/LoopVectorize/X86/x86-interleaved-accesses-masked-group.ll:46
+;DISABLED_MASKED_STRIDED-NOT: for.body:
+;DISABLED_MASKED_STRIDED:     for.end:
 
----------------
Perhaps it's better to positively CHECK-NEXT instead of these multiple CHECK-NOT's.


https://reviews.llvm.org/D53420