[PATCH] D96168: [AssumptionCache] Avoid dangling llvm.assume calls in the cache

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AssumptionCache.h:81
+  /// Set of value handles for calls of the \@llvm.assume intrinsic.
+  using AssumeHandleSet = DenseSet<AssumeHandle, DenseMapInfo<Value *>>;
+  AssumeHandleSet AssumeHandles;
----------------
Changing this list to a DenseSet introduces indeterminism in Polly's test case `ScopInfo/user_provided_assumptions.ll` and even in `basic.ll` that is changed in this patch. ScalarEvolution is known to give different result depending on the order in which elements are processed. `PredicateInfo` might emit different IR depending on the order as well.

I think we should play it save and changed it back to a `SmallVector`. Unfortunately, a SetVector instead would be of no use since the removing an element still searches the element in the vector.



================
Comment at: llvm/lib/Analysis/AssumptionCache.cpp:171
+  AC->AssumeHandles.erase(*this);
+  // 'this' now dangles!
 }
----------------
Call base method `CallbackVH::deleted()` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96168