[PATCH] D28249: Improve scheduling with branch coalescing

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

In general pretty happy with this. A few comments inline.

A few top level comments:

What performance testing have you done with this?
Looks like SPEC on ppc?
Anything else?
Same for correctness checking. You mentioned that this doesn't affect ARM and then change a thumb testcase, what's up there?

Thanks!

-eric



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


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:230
+///
+bool BranchCoalescing::analyzeBranch(CoalescingCandidateInfo &Cand) {
+  DEBUG(dbgs() << "Analyzing branch for block " << Cand.BranchBlock->getNumber()
----------------
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.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:252
+  }
+
+  // For now only consider triangles (i.e, BranchTargetBlock is set,
----------------
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.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:289-303
+  for (MachineBasicBlock *Succ : Cand.BranchBlock->successors())
+    if (Succ != Cand.BranchTargetBlock) {
+      assert(Succ && "Expecting a valid fall-through block\n");
+
+      if (!Succ->empty()) {
+        DEBUG(dbgs() << "Fall-through block contains code -- skip\n");
+        return false;
----------------
I believe the case you're looking for here is also whether or not there's a single fall through? Might be nice to reorganize the code with that in mind.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:363
+/// refer to the new block. PHI instructions in SourceMBB are placed at the
+/// beginning of TargetMBB, before existing PHI instructions.
+///
----------------
Could use an explanation of why - it confused Nemanja at first and could confuse others.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:612
+  if (SourceRegion.MustMoveUp && SourceRegion.MustMoveDown) {
+    assert(0 && "Cannot have both MustMoveDown and MustMoveUp set!");
+    DEBUG(dbgs() << "Cannot have both MustMoveDown and MustMoveUp set!");
----------------
llvm_unreachable


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:642
+
+  // Clean-up the control flow
+  // Remove SourceRegion.FallThroughBlock before transferring successors of
----------------
Nit: All comments should be complete sentences. (A few other occurrences)


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:663-666
+  // Merge FallThroughBlock
+  // Move any PHIs down to the branch-taken block
+
+  // Not necessary to merge the fall-through blocks, they should be empty!
----------------
This set of comments is very confusing.


https://reviews.llvm.org/D28249