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

[Dorit Nuzman via Phabricator via llvm-commits]

dorit marked 3 inline comments as done.
dorit added a comment.

Addressed comments. See couple responses below. Thanks!



================
Comment at: lib/Analysis/VectorUtils.cpp:940
+        << "LV: Invalidate candidate interleaved group due to gaps that "
+           "require a scalar epilogue.\n");
+    releaseGroup(Ptr);
----------------
Ayal wrote:
> May also indicate which group is being invalidated, e.g., dump along its insert pos insn.
The current printout is in the same format of the other "LV: Invalidate candidate interleaved group" printouts elsewhere, and it's nice/clear to keep it that way. So if we want to add the insert-pos insn in the printout (which is not a bad idea :-)) we should probably add it to all these occurrences, which I think is more appropriate for a separate patch. 


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1140
   bool requiresScalarEpilogue() const {
-    return InterleaveInfo.requiresScalarEpilogue();
+    return InterleaveInfo.requiresScalarEpilogue() && EpilogCreationAllowed;
   }
----------------
Ayal wrote:
> 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.
yes, I think you're right. This is a leftover from a previous version that included other changes (coming up next in a separate patch) before I noticed this bug in LV which requires invalidating groups.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1153
 
+  bool EpilogCreationAllowed = true;
+
----------------
Ayal wrote:
> 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`.
Field removed for now due to previous comment (will reappear in followup patch that adds masking support instead of epilog, with the proposed new name :-)).


================
Comment at: test/Transforms/LoopVectorize/X86/x86-interleaved-accesses-masked-group.ll:46
+;DISABLED_MASKED_STRIDED-NOT: for.body:
+;DISABLED_MASKED_STRIDED:     for.end:
 
----------------
Ayal wrote:
> Perhaps it's better to positively CHECK-NEXT instead of these multiple CHECK-NOT's.
I like that it's clear and concise exactly what this test want to check... 


https://reviews.llvm.org/D53420