[PATCH] An Assumption-Tracking Pass
Andrew Trick
atrick at apple.com
Wed Jul 30 20:20:08 PDT 2014
(1) A high-level comment, which may have already been discussed: Rather that eagerly scanning for all callbacks whenever the first assumption is registered for a function, why not do this only when the assumptions are first queried? This may not be very important but seems like a pure win, especially when we have passes that are called repeatedly but may not need to actually query the analysis.
(2) I like that you updated passes the clone instructions to invalidate the cache. But this is not robust. Unfortunately, we don't have a hook for cloning instructions (we probably should). To make up for it, I think you at least need to write a verifier for this analysis, which should be easy.
Otherwise LGTM.
================
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 {
----------------
This comment looks copy-pasted.
================
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,
----------------
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.
================
Comment at: lib/Analysis/AssumptionTracker.cpp:28
@@ +27,3 @@
+ AT->clearAssumptions(cast<Function>(getValPtr()));
+ // this now dangles!
+}
----------------
Put 'this' in quotes to clarify meaning.
http://reviews.llvm.org/D4689
More information about the llvm-commits
mailing list