[PATCH] D22291: [CFLAA] Added an initial working implementation of Andersen's analysis

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:120
@@ +119,3 @@
+    return make_range<const_valuestate_iterator>(Itr->second.begin(),
+                                                 Itr->second.end());
+  }
----------------
I'd much rather just put a FIXME that says "here's a neat optimization we may be able to do: [...]," because this sounds like an optimization that, if it subtly breaks things, can cost a nontrivial amount of debugging time. (...And it potentially makes code harder to reason about, since `reachableValueAliases` effectively returns an arbitrary subset of the reachable value aliases for a given `InstantiatedValue`)

To be clear, I think it's a valuable optimization if we can do it, but for now, I think it would be better to focus on getting simple, correct code. When we have a reasonable number of tests/features, we can always swap back to the "sorted" version and see if anything breaks.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:255
@@ +254,3 @@
+  return Ret;
+}
+
----------------
Seems that it's inlined everywhere, so rvo/nrvo would be a non-issue. Throwing a noinline on it generates the same machine code for either function body (with clang 3.7 + -O3 + no asserts -- didn't test in any other configuration); the IR differs a tiny bit, but that difference is apparently immaterial on x86. Allowing inlining, there are a few small differences in the produced asm, and stacks end up a handful of bytes bigger in a few cases, but that's about it.

If you want to check things like this in the future, here's what I did:
- `__attribute__((noinline))` on the function definition
- Generated my cmake directory with -DCMAKE_EXPORT_COMPILE_COMMANDS=Yes
- Looked for the relevant compile command inside of /path/to/build/dir/compile_commands.json, added `-DNDEBUG`, and threw `-S -emit-llvm` on it so it would hand back IR
- Grepped the resultant IR for `getNodeBelow`

:)

If you prefer the way this is written, I think the readability difference is basically zero, so I'm fine with it.


https://reviews.llvm.org/D22291