[PATCH] D78298: [LV] Invalidate cost model decisions along with interleave groups.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Sure, the updated summary's fine :-).



================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:715
+  /// groups were invalidated.
+  bool invalidateGroups() {
     InterleaveGroupMap.clear();
----------------
Simpler to early-exit at the start by "if (InterleaveGroups.empty()) return false;" and at the end "return true;"?

Still need to set RequiresScalarEpilogue to false. One would hope RequiresScalarEpilogue would always be false if there are no groups, but that requires another fix: releaseGroup() does not recompute RequiresScalarEpilogue (if it's true and the released group requiresScalarEpilogue); another option would be to set ReleaseScalarEpilogue once at the end, if any surviving group requiresScalarEpilogue.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:753
+  /// have been invalidated.
+  bool invalidateGroupsRequiringScalarEpilogue();
 
----------------
Thanks for taking care of this as well! Would be good to have a test, perhaps a version of invalidate-cm-after-invalidating-interleavegroups.ll where idx.mul is bumped by more than 7, trip count is a power of 2, under optsize, would do(?).


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1256
   RequiresScalarEpilogue = false;
+  return !DelSet.empty();
 }
----------------
(As noted earlier, cannot simply assert that DelSet is not empty, and return true.
 But at-least now RequiresScalarEpilogue is accurate ...)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4988
   if (!useMaskedInterleavedAccesses(TTI))
-    InterleaveInfo.invalidateGroupsRequiringScalarEpilogue();
+    if (InterleaveInfo.invalidateGroupsRequiringScalarEpilogue())
+      // Invalidating interleave groups also requires invalidating all decisions
----------------
nit: could fuse into a single if. OTOH, perhaps an LLVM_DEBUG in between may be useful, as in the general invalidateGroups()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78298