[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei added inline comments.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:31
+
+static cl::opt<cl::boolOrDefault>
+    EnableBranchCoalescing("enable-branch-coalesce", cl::Hidden,
----------------
echristo wrote:
> Let's have this default to off for the initial commit and then we can turn it on in a subsequent one.
okay


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:119
+///
+/// Branch Coalesce does not split blocks, it moves everything in the same
+/// direction ensuring it does not break use/definition semantics.
----------------
kbarton wrote:
> Branch Coalescing not Branch Coalesce
okay


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:230
+///
+bool BranchCoalescing::analyzeBranch(CoalescingCandidateInfo &Cand) {
+  DEBUG(dbgs() << "Analyzing branch for block " << Cand.BranchBlock->getNumber()
----------------
echristo wrote:
> Probably should be something like "canCoalesceBranch" instead of analyzeBranch. All of the analysis is being done by analyzeBranch really and this is actually figuring out whether or not we should coalesce.
true


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:252
+  }
+
+  // For now only consider triangles (i.e, BranchTargetBlock is set,
----------------
echristo wrote:
> Meta comment on the DEBUG statements. I like them, but it might be nice to give a summary of what you're analyzing here before all of the "can't do it" statements.
okay


https://reviews.llvm.org/D28249