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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

In D63981#1579704 <https://reviews.llvm.org/D63981#1579704>, @Ayal wrote:

> 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)


Ayal, thanks for paying attention to this :-). Solution your are proposing looks more solid to me. But I'm a bit worry if reporting such WAW dependencies by LoopAccessInfo will affect many other places...and possibly compile time. What do you think?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63981