[llvm] r298191 - [GVN] Fix accidental double storage of the function BasicBlock list in iterateOnFunction

[Craig Topper via llvm-commits]

Author: ctopper
Date: Sat Mar 18 13:24:41 2017
New Revision: 298191

URL: http://llvm.org/viewvc/llvm-project?rev=298191&view=rev
Log:
[GVN] Fix accidental double storage of the function BasicBlock list in iterateOnFunction

Summary:
iterateOnFunction creates a ReversePostOrderTraversal object which does a post order traversal in its constructor and stores the results in an internal vector. Iteration over it just reads from the internal vector in reverse order.

The GVN code seems to be unaware of this and iterates over ReversePostOrderTraversal object and makes a copy of the vector into a local vector. (I think at one point in time we used a DFS here instead which would have required the local vector).

The net affect of this is that we have two vectors containing the basic block list. As I didn't want to expose the implementation detail of ReversePostOrderTraversal's constructor to GVN, I've changed the code to do an explicit post order traversal storing into the local vector and then reverse iterate over that.

I've also removed the reserve(256) since the ReversePostOrderTraversal wasn't doing that. I can add it back if we thinks it important. Though it seemed weird that it wasn't based on the size of the function.

Reviewers: davide, anemet, dberlin

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D31084

Modified:
    llvm/trunk/include/llvm/ADT/PostOrderIterator.h
    llvm/trunk/lib/Transforms/Scalar/GVN.cpp

Modified: llvm/trunk/include/llvm/ADT/PostOrderIterator.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/PostOrderIterator.h?rev=298191&r1=298190&r2=298191&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/PostOrderIterator.h (original)
+++ llvm/trunk/include/llvm/ADT/PostOrderIterator.h Sat Mar 18 13:24:41 2017
@@ -268,6 +268,10 @@ inverse_post_order_ext(const T &G, SetTy
 // with a postorder iterator to build the data structures).  The moral of this
 // story is: Don't create more ReversePostOrderTraversal classes than necessary.
 //
+// Because it does the traversal in its constructor, it won't invalidate when
+// BasicBlocks are removed, *but* it may contain erased blocks. Some places
+// rely on this behavior (i.e. GVN).
+//
 // This class should be used like this:
 // {
 //   ReversePostOrderTraversal<Function*> RPOT(FuncPtr); // Expensive to create

Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=298191&r1=298190&r2=298191&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Sat Mar 18 13:24:41 2017
@@ -2155,21 +2155,12 @@ bool GVN::iterateOnFunction(Function &F)
 
   // Top-down walk of the dominator tree
   bool Changed = false;
-  // Save the blocks this function have before transformation begins. GVN may
-  // split critical edge, and hence may invalidate the RPO/DT iterator.
-  //
-  std::vector<BasicBlock *> BBVect;
-  BBVect.reserve(256);
   // Needed for value numbering with phi construction to work.
+  // RPOT walks the graph in its constructor and will not be invalidated during
+  // processBlock.
   ReversePostOrderTraversal<Function *> RPOT(&F);
-  for (ReversePostOrderTraversal<Function *>::rpo_iterator RI = RPOT.begin(),
-                                                           RE = RPOT.end();
-       RI != RE; ++RI)
-    BBVect.push_back(*RI);
-
-  for (std::vector<BasicBlock *>::iterator I = BBVect.begin(), E = BBVect.end();
-       I != E; I++)
-    Changed |= processBlock(*I);
+  for (BasicBlock *BB : RPOT)
+    Changed |= processBlock(BB);
 
   return Changed;
 }