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

Thu Jan 7 19:28:15 PST 2016

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

> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Nick Lewycky" <nlewycky at google.com>, 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>,
> reviews+D7864+public+020798531ef64731 at reviews.llvm.org
> Sent: Thursday, January 7, 2016 8:29:35 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

> Finding minimal vertex separators is np-hard in general.

> However, if we can prove the ssa graph for memoryssa (which is a
> single-phi world) is chordal, which i'm pretty sure it should be, we
> can find the minimal vertex separators in linear time:
> http://www.sciencedirect.com/science/article/pii/S0166218X98001231

> I'm pretty sure with a little modification, you can play the same
> games to find the property you are wanting.

I'll look at that, thanks! 

Also, if I'm thinking about this correctly, we can do this in O(n*log(n)) time using essentially the existing technique. We know that the separator needs to dominate the original access. So, after the initial walk, you can filter out those visited blocks that dominate the original access. Starting from the original access and walking up, the relevant phis will all be vertex separators until you reach the ones that aren't. Thus, you can find the partitioning point by binary search. At each point in the search, you do another graph search, with the suspected vertex separator removed, and see if you reach none of the aliasing defs. All of the aliasing queries should be cached at that point, so the cost is just that of walking the subgraph. 

-Hal 

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

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

-- 

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/2659b946/attachment.html>