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

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jan 8 14:01:00 PST 2017


hfinkel added inline comments.


================
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.
+
----------------
davide wrote:
> 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.
I'm worried about this too. I tried to make the code here as general as possible, without casting too wide a net, to mitigate this as much as possible.

When I first started working on this, I tried to think of a way to share the matching patterns between ValueTracking and here (perhaps with some small generalization to pick up things that LVI needs that wouldn't otherwise be covered by ValueTracking's patterns. This was the best thing I could come up with. Part of the problem is that, in ValueTracking, we're matching against specific values. Here, we want to find all potentially-matching values. If you can think of a better way, I'l all ears :-)


================
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);
----------------
davide wrote:
> I would add a comment on this matchers (and the others).
You mean like this?

  // A & B or A | B or A ^ B.



https://reviews.llvm.org/D28459





More information about the llvm-commits mailing list