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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

No objections to having this in in some form, but I agree with Hal about LVI.



================
Comment at: lib/Analysis/LazyValueInfo.cpp:384
     /// don't spend time removing unused blocks from our caches.
-    DenseSet<AssertingVH<BasicBlock> > SeenBlocks;
+    DenseSet<BasicBlock *> SeenBlocks;
 
----------------
hfinkel wrote:
> 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).
Agree.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:941
     // Get the necessary information out of the call graph and nuke the
-    // function there.
+    // function there. Also, cclear out any cached analyses.
     auto &DeadC = *CG.lookupSCC(*CG.lookup(*DeadF));
----------------
s/cclear/clear/


https://reviews.llvm.org/D29006