[PATCH] D21536: [CFLAA] Include externally-visible memory aliasing information in function summaries

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Woohoo bugfixes! Thanks for the patch.


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:747
@@ -746,3 +774,1 @@
-  // Collect StratifiedInfo for each parameter
-  SmallVector<Optional<StratifiedInfo>, ExpectedMaxArgs> ParamInfos;
   for (auto &Param : Fn.args()) {
----------------
FWIW, MSVC2013 didn't like templating on a local (yes, even though said local was `const`. I can't wait until we bump passed 2013), so I had to change this code a bit. Please rebase. :)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:755
@@ +754,3 @@
+  // the same StratifiedIndex.
+  DenseMap<StratifiedIndex, std::vector<InterfaceValue>> InterfaceMap;
+  // Insert the given parameter/return value as well as the values below it into
----------------
This still seems n^2-ish in some cases, which I believe ultimately makes this algorithm n^3 time, and n^2 storage. Imagine we have:

```
void foo(int **a, int ***b, int ****c) {
  *c = b;
  *b = a;
  int *d = *a;
}
```

That would give us one "chain" of

```
a
^
b
^
c
^
d
```

When scanning these, it looks like we'll add 4 entries for `a`, 3 entries for `b`, and 2 for `c`. It also looks like we may end up with 6 entries in cases like:

```
void foo(int ***a, int ***b) {
  int **ad = *a;
  int *add = *ad;
  int **bd = *b;
  int *bdd = *bd;
}
```

When I think we shouldn't have more than 2 (or 4, depending on how we want to handle attributes).

Assuming I'm not missing something, I think we can improve on this. AIUI, given two stratifiedset chains:

```
a
^
b  e
^  ^
c  f
^  ^
d  g
   ^
   h
```

Adding an assignment edge between b and e (or c and f, or d and g) produces the same result as adding assignment edges between all of the aforementioned pairs.

If this is correct, we can knock down the time+space complexity a bit, I think. The rough idea I have for how to do this is:

- Build a map, M, of StratifiedIndex -> vector<Value *>, where each vector is the list of Args/Return values that are in that StratifiedSet
- For each element, E, in the above map:
  - Add entries to `RetParamRelations` that represent assignment between `E.second[0]` and `E.second[1..E.second.size()]` (**)
  - For each set index S below `E.first`:
    - If we don't find a mapping in M for S, try the next lower set.
    - Otherwise, given said mapping, M', for a set N levels below E, add an entry that represents N levels of dereference between `E.second[0]` and `M'.second[0]` in `RetParamRelations`.

(** If you want, you can probably do this when building the map, and just make the map a StratifiedIndex -> Value* map. I realized this after writing the above, and am lazy :) )

With this, it looks like we'll end up with a linear number of entries (worst-case; best case, we'll have 0), and we'll end up walking N*M StratifiedSets total (N = number of args/returns, M = max(chain length))

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:758
@@ +757,3 @@
+  // InterfaceMap
+  auto AddToInterfaceMap = [this, &InterfaceMap](unsigned InterfaceIndex,
+                                                 StratifiedIndex SetIndex) {
----------------
I don't care either way, but if you want to just use `[&]` for captures in the future, you're welcome to. :)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:764
@@ +763,3 @@
+      auto &Link = Sets.getLink(SetIndex);
+      if (Link.hasBelow()) {
+        ++Level;
----------------
Nit: `if (!Link.hasBelow()) break;` seems cleaner to me

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:784
@@ +783,3 @@
+  // Populate InterfaceMap for return values
+  for (unsigned I = 0, E = RetVals.size(); I != E; ++I) {
+    auto RetInfo = Sets.find(RetVals[I]);
----------------
`for (auto *V : RetVals)`?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:785
@@ +784,3 @@
+  for (unsigned I = 0, E = RetVals.size(); I != E; ++I) {
+    auto RetInfo = Sets.find(RetVals[I]);
+    if (RetInfo.hasValue())
----------------
Can we assert that `RetVals[I]` is a pointer here? I realize that it's only meant to hold values of pointer type, but that isn't exactly locally obvious IMO. :)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:792
@@ +791,3 @@
+  // RetParamRelations
+  for (const auto &mapping : InterfaceMap) {
+    auto &Interfaces = mapping.second;
----------------
Nit: Mapping

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:792
@@ +791,3 @@
+  // RetParamRelations
+  for (const auto &mapping : InterfaceMap) {
+    auto &Interfaces = mapping.second;
----------------
george.burgess.iv wrote:
> Nit: Mapping
It looks like there are cases where this won't add the correct StratifiedAttrs to sets. Consider:

```
int **g;
void foo(int **a) {
  *a = *g;
}

void bar() {
  int *p, *p2;
  *g = p2;

  int **a = &p;
  foo(a);
  // p and p2 now alias
}
```

Because there's only arg and no return values when analyzing `foo`, `Interfaces.size()` will never be > 1, so we'll end up with no `RetParamRelations`. We need to somehow apply the attributes from the set containing `a` in `foo` to the set containing `a` in `bar`, though.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:799
@@ -802,1 +798,3 @@
+    for (size_t I = 1, E = Interfaces.size(); I < E; ++I)
+      RetParamRelations.push_back(ExternalRelation{Base, Interfaces[I]});
   }
----------------
Looks like `RetParamRelations` will hold n^2 elements, because `InterfaceMap` holds n^2 elements.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:863
@@ +862,3 @@
+    SetBuilder.add(ToVal);
+    SetBuilder.addBelowWith(FromVal, Edge.From.DerefLevel, ToVal,
+                            Edge.To.DerefLevel);
----------------
Looks like we do a linear walk of StratifiedSet chains for every iteration of this loop, so this might be n^3ish overall, assuming `RetParamRelations` contains n^2 elements?


http://reviews.llvm.org/D21536