[PATCH] An Assumption-Tracking Pass
Hal Finkel
hfinkel at anl.gov
Thu Sep 4 20:44:45 PDT 2014
----- Original Message -----
> From: "Andrew Trick" <atrick at apple.com>
> To: reviews+D4689+public+c86a7b2397dbe78c at reviews.llvm.org
> Cc: hfinkel at anl.gov, listmail at philipreames.com, chandlerc at gmail.com, "llvm commits" <llvm-commits at cs.uiuc.edu>
> Sent: Thursday, September 4, 2014 9:13:51 PM
> Subject: Re: [PATCH] An Assumption-Tracking Pass
>
>
> > On Sep 4, 2014, at 9:47 AM, hfinkel at anl.gov wrote:
> >
> > 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.
>
> I was not suggesting an incremental analysis. I was suggesting a full
> analysis per function that would be lazily computed at the first
> call to assumptions(Function). I don't think it would change the
> structure or complexity of the pass. I guess the question is, why
> does registerAssumption need to do any work if the function has not
> yet been scanned?
Ah, yes, you're absolutely right. That does indeed sound better. Thanks!
-Hal
>
> No need for further review though.. you decide.
>
> > - 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).
>
> It’s nice to avoid an analysis unless another pass will definitely
> need the result. That’s the unfortunate thing about the way analysis
> passes are normally scheduled. But in this case it’s easy. You can
> perform the analysis per function, only when needed without worrying
> about pass order.
>
> > 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).
>
> Great!
>
> -Andy
>
> >
> > ================
> > 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
> >
> >
>
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list