[PATCH] D77402: [AssumeBundles] adapt Assumption cache to assume bundles

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM, see below for some additional style changes and documentation you should apply before committing.



================
Comment at: llvm/lib/Analysis/AssumptionCache.cpp:155
+      if (!HasNonnull)
+        AffectedValues.erase(AVI);
+    }
----------------
Tyker wrote:
> jdoerfert wrote:
> > Could you explain the need for the above logic please? Is is strictly related to the index extension or a general improvement?
> > 
> > 
> > (Nit: Use a continue to reduce indention and simplify the code)
> the AffectedValues map can associate multiple assumes with a single value this is true before and after the patch although i am not sure it is reachable before the patch.
> 
> here is an example of when this change matters:
> ```
> call void @llvm.assume(i1 true) ["nonnull"(i32* %P)] ; called A
> call void @llvm.assume(i1 true) ["align"(i32* %P, i32 4)] ; called B
> ```
> with both A and B in cache both A and B will be in the AffectedValues for %P.
> 
> without this change if unregisterAssumption is called on A, all assumption of P will be invalidate even the one in B, however B will still be in the AssumeHandles (the list of all assumes in the function).
> with this change if unregisterAssumption is called on A, A will be removed (set to null) for the assumptions on V and B will not be affected.
> 
> this is not strictly related only to AssumeBundles. but it makes a difference in a situtation which is rare if possible without assume bundles.
> 
> Note: that having a nullptr in assumption is already possible since Value* are stored in WeakTrackingVH so it can be set to null if it is removed.
OK. I expected this to be a improvement not strictly related to assume bundles but I wasn't sure. You explanation makes sense to me.

---

Before you commit maybe replace `AVI != ..end()` with the opposite condition and a continue.
Add a message to the assert please.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77402/new/

https://reviews.llvm.org/D77402