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

[Jia Chen via llvm-commits]

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

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:92
@@ +91,3 @@
+  FlowToWriteOnly,
+  FlowToReadWrite,
+  // The following two states together represent S4 in the paper
----------------
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.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:377
@@ +376,3 @@
+
+#define NEXT_ASSIGN_STATE(STATE)                                               \
+  for (const auto &AssignEdge : NodeInfo->Edges)                               \
----------------
george.burgess.iv wrote:
> Can this (and the 2 below) be lambdas instead?
... I think you are right. Let me change them to lambdas.


https://reviews.llvm.org/D22429