[PATCH] An Assumption-Tracking Pass

[Philip Reames]

Er, it looks like this patch is missing some of the changes?  I don't see AT being piped through all the passes you're trying to use it in?  

Overall approach seems reasonable.

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:11
@@ +10,3 @@
+// This file contains a pass that keeps track of @llvm.assume intrinsics in
+// the functions of a module.
+//
----------------
Comment could be improved.  Specifically, why are they tracked?  What is the actually mapping?

Suggestions: pass that keeps track of @llvm.assume intrinsics found in each function of the module.  As a result, scanning for assumes is quite cheap and can be used freely by the optimizers.  

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:30
@@ +29,3 @@
+class AssumptionTracker : public ImmutablePass {
+  /// A callback value handle applied to function objects, which we use to
+  /// delete our cache of intrinsics for a function when it is deleted.
----------------
Order wise, I'd reverse this.  Interface at the top, details at the bottom.  What are most users going to want to find?

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:90
@@ +89,3 @@
+  /// Remove the cache of @llvm.assume intrinsics for the given function.
+  void clearAssumptions(Function *F);
+
----------------
Naming wise, this is used to force a recalculation, not to remove intrinsics.  Maybe a name like "forgetCachedAssumptions" might make that clearer?

================
Comment at: include/llvm/Analysis/AssumptionTracker.h:101
@@ +100,3 @@
+
+  inline assumption_range assumptions(Function *F) {
+    FunctionCallsMap::iterator I = CachedAssumeCalls.find(F);
----------------
Might be worth adding a couple of filter ranges here.  e.g. dominating_assumptions(F, I), or entry_assumptions(F).  

Putting them here would simplify implementation changes later if we wanted to tune for these.  

================
Comment at: lib/Analysis/AssumptionTracker.cpp:56
@@ +55,3 @@
+  // to our cache.
+  for (BasicBlock &B : *F)
+    for (Instruction &II : B)
----------------
I might use a inst_iterator here.  Also, I'm not a huge fan of the match notation for such trivial matches, but that's purely stylistic.  

================
Comment at: lib/Analysis/AssumptionTracker.cpp:70
@@ +69,3 @@
+  Function *F = CI->getParent()->getParent();
+  assert(F && "Cannot register @llvm.assume call not in a function");
+
----------------
Adding a check that instruction belongs to a BB might be useful.  

================
Comment at: lib/Analysis/AssumptionTracker.cpp:74
@@ +73,3 @@
+  if (I == CachedAssumeCalls.end()) {
+    I = scanFunction(F);
+    assert(I->second.count(CI) && "Function scan did not find the call");
----------------
The choice to scan here is non-obvious.  Doing so avoids a rescan latter, but is that valuable?  

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1003
@@ -1001,3 +1002,3 @@
     if (match(IIOperand, m_And(m_Value(A), m_Value(B)))) {
-      Builder->CreateCall(AssumeIntrinsic, A, II->getName());
-      Builder->CreateCall(AssumeIntrinsic, B, II->getName());
+      AT->registerAssumption(
+        Builder->CreateCall(AssumeIntrinsic, A, II->getName()));
----------------
This part seems bug prone.  Don't we already have hook into the builder which catches all adds to the worklist?  What about using that mechanism to do assumption recognition?  

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:828
@@ +827,3 @@
+  // whole function's cache.
+  AT->clearAssumptions(F);
+
----------------
I may be missing the obvious here, but where did this AT come from?  I don't see it being piped through the class.

http://reviews.llvm.org/D4689