[PATCH] D20957: [JumpThreading] Prevent dangling pointer problems in BranchProbabilityInfo

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:120
@@ -118,1 +119,3 @@
 
+  /// \brief Forget analysis results for the given basic block.
+  void eraseBlock(const BasicBlock *BB);
----------------
New code should not use `\brief`, since we have autobrief enabled for our doxygen.

================
Comment at: include/llvm/Transforms/Utils/Local.h:302
@@ -300,3 +301,3 @@
 /// Remove all blocks that can not be reached from the function's entry.
 ///
 /// Returns true if any basic block was removed.
----------------
Nit: document the new parameter (and maybe the `\p LVI` as well)?

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:643
@@ +642,3 @@
+void BranchProbabilityInfo::eraseBlock(const BasicBlock *BB) {
+  for (auto I = Probs.begin(), E = Probs.end(); I != E; ++I) {
+    auto Key = I->first;
----------------
Why do you need to do a linear search here?  Isn't `Probs` a `DenseMap<Edge, BranchProbability>`?  IOW, why not:

```
for (int i = 0; i < BB->numSuccessors(); i++)
  Probs.erase({BB, i});
```


================
Comment at: unittests/Analysis/BlockFrequencyInfoTest.cpp:58
@@ +57,3 @@
+
+  void releaseBFI() {
+    // Prevent AssetingVH from failing
----------------
Should this be called `releaseBPI`?

================
Comment at: unittests/Analysis/BlockFrequencyInfoTest.cpp:59
@@ +58,3 @@
+  void releaseBFI() {
+    // Prevent AssetingVH from failing
+    BPI->releaseMemory();
----------------
Nit: spelling.  Also add a period after the sentence.


http://reviews.llvm.org/D20957