[PATCH] D28459: Make processing @llvm.assume more efficient - Add affected values to the assumption cache

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I'm going to try this on the testcases where your previous approach regressed and review again. In the meanwhile, some early comments.



================
Comment at: include/llvm/Analysis/AssumptionCache.h:125-128
+    if (AVI == AffectedValues.end())
+      return MutableArrayRef<WeakVH>();
+
+    return AVI->second;
----------------
I prefer using the ternary operator here, but up to you.


================
Comment at: lib/Analysis/AssumptionCache.cpp:39-42
+void AssumptionCache::updateAffectedValues(CallInst *CI) {
+  // Note: This code must be kept in-sync with the code in
+  // computeKnownBitsFromAssume in ValueTracking.
+
----------------
I see why you need this, but, can we think of a way of sharing (at least partially) the code between this and `ValueTracking`?
I'm very  worried the two implementations going out of sync quickly.


================
Comment at: lib/Analysis/AssumptionCache.cpp:78-82
+        if (match(V,
+                  m_CombineOr(m_And(m_Value(A), m_Value(B)),
+                    m_CombineOr(m_Or(m_Value(A), m_Value(B)),
+                                m_Xor(m_Value(A), m_Value(B)))))) {
+          AddAffected(A);
----------------
I would add a comment on this matchers (and the others).


https://reviews.llvm.org/D28459