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

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!


================
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
----------------
Why are we now starting from 1?

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

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:356
@@ +355,3 @@
+
+#define MEMALIAS_PROPAGATE(SRC, DST)                                           \
+  if (Mapping.second.test(static_cast<size_t>(MatchState::SRC)))               \
----------------
Can this be a lambda instead?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:377
@@ +376,3 @@
+
+#define NEXT_ASSIGN_STATE(STATE)                                               \
+  for (const auto &AssignEdge : NodeInfo->Edges)                               \
----------------
Can this (and the 2 below) be lambdas instead?


https://reviews.llvm.org/D22429