[PATCH] An Assumption-Tracking Pass
hfinkel at anl.gov
hfinkel at anl.gov
Thu Sep 4 09:47:16 PDT 2014
Regarding the scanning-on-register, the rationale is that:
- It keeps the implementation simple. This way we have an invariant: If a function is in the cache then its set of assumptions is complete. If we don't scan on registration then we could have partial sets in the cache, and we need to keep track of that. Obviously, that could be done, but I'd rather not add the extra complexity unless there is a clear benefit.
- Functions with assumptions will likely be scanned at some point, and because the pass is never invalidated (the analysis is immutable), this is not wasted work (except perhaps that the cache in invalidated when we inline a callee, but that's a separate issue that will be fixed regardless).
A verification function has been added; because the pass manager currently does not verify immutable passes, I added a call to the verification inside the pass's finalization function (this does not provide a huge amount of coverage, but it is something).
================
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.
+//
----------------
reames wrote:
> 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.
Done.
================
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.
----------------
reames wrote:
> Order wise, I'd reverse this. Interface at the top, details at the bottom. What are most users going to want to find?
Unfortunately, this required a number of forward declarations, and I'm not sure it is worthwhile. Many other analysis classes have their interface at the bottom too for this reason.
================
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;
----------------
chandlerc wrote:
> I wouldn't bother with the 'AT' prefix for a nested class.
Done.
================
Comment at: include/llvm/Analysis/AssumptionTracker.h:41-42
@@ +40,4 @@
+
+ /// A callback value handle applied to function objects, which we use to
+ /// delete our cache of intrinsics for a function when it is deleted.
+ class ATCallCallbackVH : public CallbackVH {
----------------
atrick wrote:
> This comment looks copy-pasted.
Indeed. Fixed.
================
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();
+ }
----------------
hfinkel wrote:
> chandlerc wrote:
> > Why do you support building these for instructions without parents?
> Hrmm... maybe this is not needed, IIRC it comes from.... Currently, in AssumptionTracker::ATCallCallbackVH::deleted() I have this:
> I->second.erase(cast<CallInst>(getValPtr()))
> this causes a new temporary ATCallCallbackVH to be created from getValPtr() in order to remove *this from the map. getValPtr() has already been removed from its parent function, and so it has no parent, which is why this was needed.
Yes, can be cleaned up (and is in the next version).
================
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,
----------------
chandlerc wrote:
> atrick 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.
Done.
================
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;
+
----------------
chandlerc wrote:
> 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.
Done.
Regarding the small-size-optimized container, we have a SmallDenseMap, however it is not clear to me that the key-set is actually going to be small. It will almost certainly contain all non-empty functions in the module. Is that small?
================
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);
+
----------------
reames wrote:
> Naming wise, this is used to force a recalculation, not to remove intrinsics. Maybe a name like "forgetCachedAssumptions" might make that clearer?
Done.
================
Comment at: include/llvm/Analysis/AssumptionTracker.h:95-97
@@ +94,5 @@
+
+ // ---------------------------------------------------------------------------
+ // Assumption Iterator interface...
+ //
+ typedef CallHandleSet::iterator assumption_iterator;
----------------
chandlerc wrote:
> I don't think these section comments are really adding much value. It's all pretty clear.
Removed.
================
Comment at: include/llvm/Analysis/AssumptionTracker.h:101
@@ +100,3 @@
+
+ inline assumption_range assumptions(Function *F) {
+ FunctionCallsMap::iterator I = CachedAssumeCalls.find(F);
----------------
reames wrote:
> 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.
I agree, but let's do this as-needed.
================
Comment at: include/llvm/Analysis/AssumptionTracker.h:118
@@ +117,3 @@
+ void releaseMemory() override {
+ CachedAssumeCalls.clear();
+ }
----------------
chandlerc wrote:
> Does this actually release memory though?
Only sometimes ;) -- shrink_and_clear() always does.
================
Comment at: lib/Analysis/AssumptionTracker.cpp:28
@@ +27,3 @@
+ AT->clearAssumptions(cast<Function>(getValPtr()));
+ // this now dangles!
+}
----------------
atrick wrote:
> Put 'this' in quotes to clarify meaning.
Done.
================
Comment at: lib/Analysis/AssumptionTracker.cpp:56
@@ +55,3 @@
+ // to our cache.
+ for (BasicBlock &B : *F)
+ for (Instruction &II : B)
----------------
reames wrote:
> 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.
I'm not sure that buys us anything, the range-based for loop is pretty simple.
================
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");
+
----------------
reames wrote:
> Adding a check that instruction belongs to a BB might be useful.
Done.
================
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");
----------------
reames wrote:
> The choice to scan here is non-obvious. Doing so avoids a rescan latter, but is that valuable?
Documented. (Also see general comment).
================
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()));
----------------
reames wrote:
> 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?
Thanks for pointing this out, I did not realize that IRBuilder had that hook. Done.
================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:828
@@ +827,3 @@
+ // whole function's cache.
+ AT->clearAssumptions(F);
+
----------------
reames wrote:
> I may be missing the obvious here, but where did this AT come from? I don't see it being piped through the class.
Yep; thus the comment about the patch interdependencies when I posted.
http://reviews.llvm.org/D4689
More information about the llvm-commits
mailing list