[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]

----- Original Message -----

> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: reviews+D7864+public+020798531ef64731 at reviews.llvm.org
> Cc: "Nick Lewycky" <nlewycky at google.com>, "Hal Finkel"
> <hfinkel at anl.gov>, anemet at apple.com, "Pete Cooper"
> <peter_cooper at apple.com>, "vikram tarikere"
> <vikram.tarikere at gmail.com>, "Félix Cloutier" <felixcca at yahoo.ca>,
> "David Majnemer" <david.majnemer at gmail.com>, "James Molloy"
> <james.molloy at arm.com>, "Philip Reames" <listmail at philipreames.com>,
> "Sanjoy Das" <sanjoy at playingwithpointers.com>, "llvm-commits"
> <llvm-commits at lists.llvm.org>
> Sent: Thursday, January 7, 2016 6:52:57 PM
> Subject: Re: [PATCH] D7864: This patch introduces MemorySSA, a
> virtual SSA form for memory.Details on what it looks like are in
> MemorySSA.h

> On Thu, Jan 7, 2016 at 4:48 PM, Daniel Berlin < dberlin at dberlin.org >
> wrote:

> > On Thu, Jan 7, 2016 at 4:27 PM, Hal Finkel < hfinkel at anl.gov >
> > wrote:
> 

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

> > So, note, in the above case, there *must* be a memory phi in the
> > block of starting access, merging the two memory accesses. We
> > should
> > say that is the clobbering access.
> 

> > But you are correct, that this version, in the middle of
> > refactoring,
> > did not finish this rule correctly. The original algorithm only
> > walked past a phi if all paths away from the phi reached the same
> > clobbering access.
> 
That makes sense, thanks! 

> > This one should do the same.
> 
Yep, looks the same in this regard. 

> (You can see this in the upwardsdfswalk code that got removed:

Indeed; the version from April recursed on phis to enforce this condition. 

> // All the phi arguments should reach the same point if we can bypass
> - // this phi. The alternative is that they hit this phi node, which
> - // means we can skip this argument.
> - if (FirstDef && (CurrentPair.first != PHIPair.first &&
> - FirstDef != CurrentPair.first)) {
> - ModifyingAccess = CurrAccess;
> - break;
> - } else if (!FirstDef) {
> - FirstDef = CurrentPair.first;
> )

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

> > Again, we will have a memory phi node here, and we will say the phi
> > node is the clobbering access. We will walk one step from the phi,
> > hit a, and say "we have a clobbering access."
> 
> and by here i mean, you will have a memoryphi node in the block of
> startingaccess, and we should say that is the clobbering node.

Yep. 

Out of curiosity, is there an efficient algorithm to solve the problem optimally? I suppose I could phrase it like this: Given some digraph G, a vertex S, and a set of verticies F, compute the set of vertex seperators between S and F, or more specifically, the vertex separator between S and F that has the largest distance from S. 

Thanks again, 
Hal 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160107/617b9805/attachment.html>