[llvm-commits] [PATCH] [msan] Remove unreachable bb's

[Evgeniy Stepanov]

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:732
@@ +731,3 @@
+/// function's entry.
+void llvm::RemoveUnreachableBlocks(Function &F) {
+  SmallPtrSet<BasicBlock*, 128> Reachable;
----------------
A comment in the header file is following the coding standards. Do we need another one in .cpp at all? It's probably good for reading through the code, but in the doxygen documentation they are joined together, making it look weird. See what happened with llvm::FindAllocaDbgDeclare documentation here:
http://llvm.org/docs/doxygen/html/namespacellvm.html#aabf35a012f84ad0dab49cd5b7dec7965

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:748
@@ +747,3 @@
+  assert(Reachable.size() < F.size());
+  for (Function::iterator BB = ++F.begin(); BB != F.end();)
+    if (!Reachable.count(BB)) {
----------------
Chandler Carruth wrote:
> Chandler Carruth wrote:
> > ++ is confusing (and may be risky) here -- what guarantee do you have that begin() returns a temporary which can be mutated?
> > 
> > I would use llvm::next(F.begin()), this is also the more idiomatic way to do this.
> > 
> > Also, this should be written using the idiomatic LLVM loop pattern:
> > 
> >   for (Function::iterator I = llvm::next(F.begin()), E = F.end(); I != E;)
> > 
> > The end iterator is stable even across basic blocks being removed.
> I still think using I or BI or BBI, or anything with the I in it to indicate that this is an iterator would be good. I would personally use 'I' and rename the variable below from 'I' to something else.
OK, I'll change "BB" to "I" and "I" to "Inst".

================
Comment at: lib/Transforms/Utils/Local.cpp:971
@@ +970,3 @@
+bool llvm::removeUnreachableBlocks(Function &F) {
+  SmallPtrSet<BasicBlock*, 128> Reachable;
+  SmallVector<BasicBlock*, 128> Worklist;
----------------
Benjamin Kramer wrote:
> 128 is really large, SmallPtrSet falls back to a linear search instead of hashing as long as the set is "small".
> 
> The sweet spot for SmallPtrSets is usually at about 8 or 16 small members.
Good point, 16 it is.
For the vector, 128 still seems like a good "small" size.


http://llvm-reviews.chandlerc.com/D208