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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:715
+  /// groups were invalidated.
+  bool invalidateGroups() {
     InterleaveGroupMap.clear();
----------------
Ayal wrote:
> Ayal wrote:
> > 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.
> Actually releaseGroup() does not need to recompute RequiresScalarEpilogue, because the latter is indeed set according to surviving groups only, that are not released later.
I've added an assertion that empty groups implies RequiresScalarEpilogue == false.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:753
+  /// have been invalidated.
+  bool invalidateGroupsRequiringScalarEpilogue();
 
----------------
Ayal wrote:
> 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(?).
This is actually not required, as the groups are invalidated before any cost-modeling decisions are taken. I've added a test case that should crash otherwise, as suggested.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1256
   RequiresScalarEpilogue = false;
+  return !DelSet.empty();
 }
----------------
Ayal wrote:
> (As noted earlier, cannot simply assert that DelSet is not empty, and return true.
>  But at-least now RequiresScalarEpilogue is accurate ...)
(from above)
> Actually releaseGroup() does not need to recompute RequiresScalarEpilogue, because the latter is indeed set according to surviving groups only, that are not released later.


Right, it should always be up-to-date, right? in invalidateGroups all groups are removed and it is set to false. In invalidateGroupsRequiringScalarEpilogue, all groups requiring a scalar epilogue are removed and it is set to false as well. I've added an assertion.


================
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
----------------
Ayal wrote:
> nit: could fuse into a single if. OTOH, perhaps an LLVM_DEBUG in between may be useful, as in the general invalidateGroups()?
There's no need to invalidate anything CM related, as nothing's computed yet. I've added an assertion to that effect and a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78298