[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