[PATCH] D31084: [GVN] Fix accidental double storage of the function BasicBlock list in iterateOnFunction

[Craig Topper via Phabricator via llvm-commits]

craig.topper created this revision.

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.


https://reviews.llvm.org/D31084

Files:
  lib/Transforms/Scalar/GVN.cpp


Index: lib/Transforms/Scalar/GVN.cpp
===================================================================
--- lib/Transforms/Scalar/GVN.cpp
+++ lib/Transforms/Scalar/GVN.cpp
@@ -2159,15 +2159,10 @@
   // 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.
-  ReversePostOrderTraversal<Function *> RPOT(&F);
-  for (ReversePostOrderTraversal<Function *>::rpo_iterator RI = RPOT.begin(),
-                                                           RE = RPOT.end();
-       RI != RE; ++RI)
-    BBVect.push_back(*RI);
+  std::copy(po_begin(&F), po_end(&F), std::back_inserter(BBVect));
 
-  for (std::vector<BasicBlock *>::iterator I = BBVect.begin(), E = BBVect.end();
+  // Need to go through the vector in reverse.
+  for (std::vector<BasicBlock *>::reverse_iterator I = BBVect.rbegin(), E = BBVect.rend();
        I != E; I++)
     Changed |= processBlock(*I);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D31084.92158.patch
Type: text/x-patch
Size: 1009 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170317/56cfb395/attachment.bin>