[PATCH] D22429: [CFLAA] Teach CFLAndersAliasAnalysis to discern reads from writes

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:86
@@ +85,3 @@
+  // The following state represents S1 in the paper
+  FlowFromReadOnly = 1,
+  // The following two states together represent S2 in the paper
----------------
grievejia wrote:
> george.burgess.iv wrote:
> > Why are we now starting from 1?
> There was a state "FlowFromNoReadWrite" which was labelled 0. It was supposed to be the starting state. Later I found that we don't really need it since we do not store identity mapping (X, X) in ReachabilitySet, so I removed that state.
> 
> I thought the starting index doesn't really matter that much. I'm OK with any choice here.
The only reason I asked is because we're using these as indices into a bitset. 7 bits vs 8 bits is a wash; I was just making sure we weren't sneakily using the `bits[0]` bit in some way. :)

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:92
@@ +91,3 @@
+  FlowToWriteOnly,
+  FlowToReadWrite,
+  // The following two states together represent S4 in the paper
----------------
grievejia wrote:
> george.burgess.iv wrote:
> > Out of curiosity, do we gain anything from having `FlowToWriteOnly` on something that already has `FlowToReadWrite` on it?
> > 
> > Specifically, given:
> > 
> > ```
> > ReachabilitySet R = whatever;
> > R.insert(FromVal, ToVal, FlowToReadWrite);
> > R.insert(FromVal, ToVal, FlowToWriteOnly);
> > ```
> > 
> > Is the second insert at all meaningful to us? If not, can we add a FIXME or TODO somewhere noting that we may be able to do less propagations if we check for this later?
> > 
> > (Same question for `FlowFromMemAliasNoReadWrite` vs `FlowFromMemAliasReadOnly`, and `FlowToMemAliasWriteOnly` vs `FlowToMemAliasReadWrite`)
> The state FlowToWriteOnly does carry important information: if x is reachable from y at FlowToWriteOnly state, we know that y is (transitively) assigned to x. If x is reachable from y at FlowToReadWrite state, then there's a third value z which gets assigned to both x and y. The distinction here is basically what motivates this patch.
Ahh, now that makes sense. Thanks for clarifying!


https://reviews.llvm.org/D22429