[PATCH] D63981: [LV] Avoid building interleaved group in presence of WAW dependency

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

>From the SUMMARY above:

> I don't think we actually need to do any additional book keeping (as suggested in D57180 <https://reviews.llvm.org/D57180>). Current algorithm already examines all pairs and in case of dependence will invalidate interleaving group. I think the real issue is WAW dependence (on the same iteration) is not collected by LoopAccessInfo in the first place and as a result canReorderMemAccessesForInterleavedGroups returns wrong answer. The fix is to explicitly check for WAW dependence in canReorderMemAccessesForInterleavedGroups.

Nice catch! Agree about the real issue being a deficiency of LoopAccessInfo, and that it's better to treat all interleave-group-preventing dependencies the same, w/o dedicating special treatment for same-iteration WAW dependences. Suggest to fix this real issue at its core.

LoopAccessInfo does collect the 6 same-iteration WAR Forward dependences between each of the 3 loads and their 2 subsequent same-address stores, in the testcase below, but these do not interfere with interleave-grouping the stores. So it's perfectly capable of collecting same-iteration WAW Forward dependences as well. (LoopAccessInfo should also already collect any same-iteration RAW dependences, although such cases would hopefully be optimized away.)

The reason why LoopAccessInfo does not collect same-iteration WAW dependences is due to the way areDepsSafe() traverses pairs of DependentAccesses, held in EquivalenceClasses<MemAccessInfo> which clusters stores-to-same-address together (and load-to-same-address together, but separately from the stores). One way of fixing this may be:

  diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
  index 36bd9a8b7ea..a3f40826861 100644
  --- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
  +++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
  @@ -1641,13 +1641,21 @@ bool MemoryDepChecker::areDepsSafe(DepCandidates &AccessSets,
       // Check every access pair.
       while (AI != AE) {
         Visited.insert(*AI);
  -      EquivalenceClasses<MemAccessInfo>::member_iterator OI = std::next(AI);
  +      bool AIIsWrite = AI->getInt();
  +      // Check loads only against next equivalent class, but stores also against
  +      // other stores in the same equivalence class - to the same address.
  +      EquivalenceClasses<MemAccessInfo>::member_iterator OI =
  +          (AIIsWrite ? AI : std::next(AI));
         while (OI != AE) {
           // Check every accessing instruction pair in program order.
           for (std::vector<unsigned>::iterator I1 = Accesses[*AI].begin(),
                I1E = Accesses[*AI].end(); I1 != I1E; ++I1)
  -          for (std::vector<unsigned>::iterator I2 = Accesses[*OI].begin(),
  -               I2E = Accesses[*OI].end(); I2 != I2E; ++I2) {
  +          // Scan all accesses of another equivalence class, but only the next
  +          // accesses of the same equivalent class.
  +          for (std::vector<unsigned>::iterator
  +                   I2 = (OI == AI ? std::next(I1) : Accesses[*OI].begin()),
  +                   I2E = (OI == AI ? I1E : Accesses[*OI].end());
  +               I2 != I2E; ++I2) {
               auto A = std::make_pair(&*AI, *I1);
               auto B = std::make_pair(&*OI, *I2);

(could possibly save dependences by recording only those between pairs of adjacent stores to same address, relying on transitivity)



================
Comment at: include/llvm/Analysis/VectorUtils.h:612
+                        ? cast<StoreInst>(Src)->getPointerOperand()
+                        : cast<LoadInst>(Src)->getPointerOperand();
+      auto SinkPtr = isa<StoreInst>(Sink)
----------------
Src and Sink should only be StoreInst's here, right?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63981