[PATCH] D7864: This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1124
@@ +1123,3 @@
+  auto CurrAccessPair = UpwardsBFSWalkAccess(StartingAccess, Prev, Q);
+  // Either we will have found something that conflicts with us, or we will have
+  // hit the liveOnEntry. Check for liveOnEntry.
----------------
This algorithm in getClobberingMemoryAccess does not seem correct. If we have this:

     [ entry ]
         |
        ...
         |
     (clobbering access - a)
        ...
    /          \
  ...         (clobbering access - b)
   |              |
  ...           ....
   \              /
          ...
   (starting access)

UpwardsBFSWalkAccess seems to walk upward until it finds its first clobbering access, and returns that when found. In the case above, if we find clobbering access (b) first, then we're okay, because we'll pick some dominating def farther down the path which is on all paths to both dominating accesses.

But, if we hit clobbering access (a) first, then we'll accept that (incorrectly) as the new final access, because it dominates the original. However, not all paths to all aliasing defs terminate (or pass through) that access.

I think that you can continue the walk so that you search the entire subgraph to make sure that all paths terminate at the same aliasing def, and only in that case, use it (and, in that case, you know it must dominate).

My attempts to think up a good solution to the more-general problem have thus-far been stymied by this situation:

  [entry]
    |
  .....
  (clobbering access - b)
   |
  ....   ________________________________
    \   /                                                               |
     (x)                                                               |
   ......                                                               |
      |                                                                 |
      |    ______________________                 |
       \   /                                        |                   |
  (starting access)                        |                   |
       ...                                           |                   |
   (clobbering access - a)              |                   |
      ...                                            |                   |
      | |                                            |                   |
      | |______________________|                   |
      |                                                                  |
      |_________________________________|

The idea is that the block with the starting access also later has a clobbering access (a). This block is one of its own predecessors, but that's not the only path from clobbering access (a) back to the starting access. And furthermore, the other path flows through other blocks that do properly dominate the starting access. Thus, even if you were to pick a def (or phi) on the path from the starting access through its dominating predecessor, say at (x), and even though there are paths to all aliasing defs along that route, you'd still be wrong because there exists a path to clobbering access (a) that does not contain (x) (the path following the inner-loop backedge).

Thoughts? (am I off base here?)



http://reviews.llvm.org/D7864