[PATCH] D28249: Improve scheduling with branch coalescing

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

Bunch of inline comments.

Thanks!

-eric



================
Comment at: include/llvm/CodeGen/Passes.h:406
+
+  /// Branch Coalescing - combine basic blocks guarded by the same branch
+  extern char &BranchCoalescingID;
----------------
Nit: Complete sentences in comments please.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:378
+  MachineBasicBlock::instr_iterator I = MBB.instr_begin(), E = MBB.instr_end();
+  while (I != E && !I->isPHI())
+    ++I;
----------------
lei wrote:
> lei wrote:
> > nemanjai wrote:
> > > lei wrote:
> > > > nemanjai wrote:
> > > > > Not that it makes a difference computationally, but perhaps for consistency:
> > > > > ```
> > > > > for (MachineBasicBlock::instr_iterator Instr : MBB.instrs())
> > > > >   if (Instr->isPHI())
> > > > >     return Instr;
> > > > >   return MBB.instr_end();
> > > > > ```
> > > > > 
> > > > > Also, I'm not sure what the style guide says regarding the use of `auto` there, but perhaps instead of `MachineBasicBlock::instr_iterator`, it should be `auto`.
> > > > > 
> > > > > Finally, if every use of this function will just use `begin()` instead of `end()` when there are no existing PHI's, perhaps it would make sense to actually return `begin()` if there are no PHI's in the MBB.
> > > > Can begin() also be a PHI?  If not, then that is okay, if it can then it is better to return end() so that we know there are no PHIs in this block.
> > > Are you referring to some potential future use of this function? Because this result isn't currently used to determine whether the block has any PHI's. But I suppose you're right it is potentially dangerous for it to return a valid iterator when there are no PHI's.
> > > 
> > > Also, the work this function does just seems like overkill. It seems like you should only be checking instructions between `MBB.instr_begin()` and `MBB.getFirstNonPHI()`. Why does that not suffice?
> > > 
> > > In any case, considering this function is only used once and in many cases it'll return a value that you're just going to discard and use the `begin()` pointer, I don't think this function should exist. Just find the insert location in the function where you need it.
> > will remove function and add loop to location where it's being used.
> Actually according to the llvm lang ref, PHI instructions must be first in a basic block.  Will remove this function and replace the function call below with MBB->begin()
I don't think this has ever been changed, but I'm not against a separate function. That said, doesn't transferSuccessorsAndUpdatePHIs already do this?

Also, this loop needs documentation :)


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:393
+                                         MachineBasicBlock *ToMBB) {
+  MachineBasicBlock::iterator InsertLoc = getFirstPHI(*ToMBB);
+
----------------
lei wrote:
> nemanjai wrote:
> > lei wrote:
> > > nemanjai wrote:
> > > > Can you please state why something like `MachineBasicBlock::SkipPHIsLabelsAndDebug()` cannot be used instead of this function (thereby eliminating the need for that function altogether)?
> > > MachineBasicBlock::SkipPHIsLabelsAndDebug() returns the iterator to the first NON PHI, non-label instruction. We want the iterator to the first PHI node in a MBB.  I don't see any function within that class that returns the iterator to the first PHI.
> > OK, that makes sense. I suppose you want to make sure that you skip anything that comes between `begin()` and the first PHI node in the `ToMBB`.
> > 
> > But I guess what I'm confused about is exactly what this function is meant to do. It appears that it will do the following:
> > - Iterate over all the PHI nodes in MBB called `FromMBB`
> > - If there are any PHI nodes that refer to the same MBB, they'll be updated so they refer to the MBB called `ToMBB`
> > - Then each of those PHI nodes will be moved to MBB called `ToMBB` before any existing PHI nodes in `ToMBB`
> > 
> > However, what happens to any PHI nodes in `ToMBB` that refer to `FromMBB`? They don't need to be updated?
> This function moves PHI nodes in the FromMBB to the beginning of the PHI block in ToMBB.  If any PHI nodes in ToMBB reference registers defined in PHI nodes in FromMBB, the PHI nodes in FromMBB can not be moved to ToMBB since we can not infer order in the PHI node execution.
Should probably assert this somewhere.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:352
+///
+/// Moves ALL PHI instructions in SourceMBB to begining of TargetMBB 
+/// and update them to refer to the new block.  PHI node ordering 
----------------
"beginning"


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:367
+  if (MI == ME) {
+    DEBUG(dbgs() << "SourceMBB contain no PHI instructions.\n");
+    return;
----------------
"contains"


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:392-393
+///
+/// An MI instruction can be moved to beginning of the TargetMBB if there are no
+/// PHI's in the TargetMBB that use what MI defines.
+///
----------------
This seems like an odd comment.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:404
+///
+bool BranchCoalescing::canMoveTo(const MachineInstr &MI,
+                                 const MachineBasicBlock &TargetMBB,
----------------
I think it makes just as much sense (and is more readable) if you split the function into canMoveToBeginning and canMoveToEnd and just drop the boolean parameter.

It'll also make it easier to split the docs.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:453-466
+  if (TargetRegion.BranchTargetBlock != SourceRegion.BranchBlock)
+    err_msg = "Expecting SourceRegion to immediately follow TargetRegion";
+  else if (!MDT->dominates(TargetRegion.BranchBlock, SourceRegion.BranchBlock))
+    err_msg = "Expecting TargetRegion to dominate SourceRegion";
+  else if (!MPDT->dominates(SourceRegion.BranchBlock, TargetRegion.BranchBlock))
+    err_msg = "Expecting SourceRegion to post-dominate TargetRegion";
+  else if (!TargetRegion.FallThroughBlock->empty() ||
----------------
Possible you just want to replace this with a bunch of llvm_unreachables?


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:479
+///
+/// The preference is to move instructions down, to the
+/// beginning of the SourceRegion.BranchTargetBlock. This is not possible if any
----------------
Why?


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:487
+///
+/// Note that there is no analysis for moving instructions past the fall-through
+/// blocks because they are assumed to be empty. If they are not empty, then
----------------
Sounds like an assert.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:516
+        }
+        if (Use.isPHI() && Use.getParent() == SourceRegion.BranchTargetBlock) {
+          DEBUG(dbgs() << "PHI " << *I << " defines register used in another "
----------------
Seems like you want to swap these conditionals?


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:534
+    }
+    if (!canMoveTo(*I, *TargetRegion.BranchBlock, false)) {
+      DEBUG(dbgs() << "Instruction " << *I
----------------
else if? Otherwise what happens?


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:757
+  case cl::BOU_UNSET:
+    return TFI->enableBranchCoalescing();
+  case cl::BOU_TRUE:
----------------
This was to be turned on in a subsequent patch. Remove this and the ppc support please :)


https://reviews.llvm.org/D28249