[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei marked an inline comment as done.
lei added inline comments.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:655
+  // Move any PHIs down to the branch-taken block
+  // It is not necessary to merge the fall-through blocks because they are
+  // empty!
----------------
nemanjai wrote:
> lei wrote:
> > nemanjai wrote:
> > > Where is the assert to ensure this?
> > In analyzeBranch(), line 297, we do an early exit if the FallThroughBlock contain code.
> Sure. Now there is. I'd still prefer an assert. If someone in the future feels that `analyzeBranch()` doesn't need that early exit any longer, they'll need to ensure they account for this assert and why it's in this function.
okay


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:729
+
+      if (MergedCandidates) {
+        DEBUG(dbgs() << "Function after merging: "; MF.dump(); dbgs() << "\n");
----------------
nemanjai wrote:
> Setting a bool to true and then using it in an if statement without any statements that could modify it in between is redundant.
> Besides, I think it's overly verbose to dump the entire function after every merge - I'd imagine that a dump after all the merging is done would suffice.
For debug purposes, it would be nice to see the stages of merging... 
Will remove the if statement.


https://reviews.llvm.org/D28249