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

[Tyker via Phabricator via llvm-commits]

Tyker marked an inline comment as done.
Tyker added inline comments.


================
Comment at: llvm/lib/Analysis/AssumptionCache.cpp:155
+      if (!HasNonnull)
+        AffectedValues.erase(AVI);
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77402