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

[Davide Italiano via Phabricator via llvm-commits]

davide 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.
+
----------------
hfinkel wrote:
> 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 :-)
Yeah, unfortunately, I don't think there's an easy way to generalize. The best we can do here is trying to be very careful when reviewing new changes (and I think your comment in ValueTracking is pretty much needed to make sure we don't forget about this in a month or two).


================
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);
----------------
hfinkel wrote:
> 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.
> 
Yes, exactly.


https://reviews.llvm.org/D28459