[PATCH] An Assumption-Tracking Pass

[Chandler Carruth]

Generally I agree with Andy's high-level comment about populating the cache lazily. A few minor comments below. Otherwise, I think this makes sense given the current pass infrastructure. I look forward to this kind of thing getting *much* easier to design in the new PM world.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:32
@@ +31,3 @@
+  /// delete our cache of intrinsics for a function when it is deleted.
+  class ATFunctionCallbackVH : public CallbackVH {
+    AssumptionTracker *AT;
----------------
I wouldn't bother with the 'AT' prefix for a nested class.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:56-58
@@ +55,5 @@
+        : CallbackVH(I), AT(AT), F(nullptr) {
+        if (I != DMI::getEmptyKey() && I != DMI::getTombstoneKey() &&
+            I->getParent())
+          F = I->getParent()->getParent();
+      }
----------------
Why do you support building these for instructions without parents?

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:78-79
@@ +77,4 @@
+  typedef DenseSet<ATCallCallbackVH, ATCallCallbackVH::DMI> CallHandleSet;
+  // FIXME: DenseMap is not thread safe; if we ever run function passes
+  // concurrently, this will need to be changed!
+  typedef DenseMap<ATFunctionCallbackVH, CallHandleSet,
----------------
Andrew Trick wrote:
> I don't like this FIXME because it implies a design for concurrency that has not even been formally proposed and would require FIXMEs all over the code.
Agreed.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:80-82
@@ +79,5 @@
+  // concurrently, this will need to be changed!
+  typedef DenseMap<ATFunctionCallbackVH, CallHandleSet,
+                   DenseMapInfo<Value *>> FunctionCallsMap;
+  FunctionCallsMap CachedAssumeCalls;
+
----------------
Rather than storing the CallHandleSets by value, kep a mapping to unique_ptr<>s to them? That would in turn make it more reasonable to use a small-size optimized container in addition to reducing the size of the top level map.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:95-97
@@ +94,5 @@
+
+  // ---------------------------------------------------------------------------
+  // Assumption Iterator interface...
+  //
+  typedef CallHandleSet::iterator assumption_iterator;
----------------
I don't think these section comments are really adding much value. It's all pretty clear.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:118
@@ +117,3 @@
+  void releaseMemory() override {
+    CachedAssumeCalls.clear();
+  }
----------------
Does this actually release memory though?

http://reviews.llvm.org/D4689