[PATCH] D29006: [PH] Remove all use of AssertingVH from members of analysis results.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:603
   struct ExitNotTakenInfo {
-    AssertingVH<BasicBlock> ExitingBlock;
+    BasicBlock *ExitingBlock;
     const SCEV *ExactNotTaken;
----------------
This seems relatively safe. If you remove a loop's exiting block, you'll need to call SE->forgetLoop(L).


================
Comment at: lib/Analysis/LazyValueInfo.cpp:384
     /// don't spend time removing unused blocks from our caches.
-    DenseSet<AssertingVH<BasicBlock> > SeenBlocks;
+    DenseSet<BasicBlock *> SeenBlocks;
 
----------------
This seems less obvious; once you remove any blocks that LVI might have visited, if you add any other blocks, the analysis becomes instantly invalid in a subtle way for all other queries (i.e. subject to allocator-reuse-triggered non-determinism bugs), even for completely-unrelated parts of the function, even if you've proven the removed block dynamically unreachable, etc.

A callback handle here that removes the cached values if you remove the block would add value here in making this much less fragile. Even though flawed, I'm concerned that the asserting VH here is adding value by forcefully reminding us of this fragility. Fixing this "for real" seems straightforward (just like we do in SE in other cases).


https://reviews.llvm.org/D29006