[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei added inline comments.


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


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:378
+  MachineBasicBlock::instr_iterator I = MBB.instr_begin(), E = MBB.instr_end();
+  while (I != E && !I->isPHI())
+    ++I;
----------------
echristo wrote:
> 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 :)
It's because here we only want to move the PHIs from SourceMBB to TargetMBB.  SourceMBB will be deleted later.  We don't want to transfer the successor info here.  Control flow need to be updated once we have deleted SourceMBB.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:393
+                                         MachineBasicBlock *ToMBB) {
+  MachineBasicBlock::iterator InsertLoc = getFirstPHI(*ToMBB);
+
----------------
echristo wrote:
> 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.
We don't assert cause we first check one direction (canMoveToBeginning) and then check the other direction (canMoveToEnd).  If both fails, we just don't move the instruction and move to investigate the next one.


================
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.
+///
----------------
echristo wrote:
> This seems like an odd comment.
A MI instruction can only be moved to TargetMBB if there are no uses of it within the TargetMBB's PHI nodes. Will reword the comment.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:404
+///
+bool BranchCoalescing::canMoveTo(const MachineInstr &MI,
+                                 const MachineBasicBlock &TargetMBB,
----------------
echristo wrote:
> 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.
agreed.


================
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() ||
----------------
echristo wrote:
> Possible you just want to replace this with a bunch of llvm_unreachables?
yup


================
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
----------------
echristo wrote:
> Why?
Maybe preference is the wrong word here... this is by design.  Will update the doc


================
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
----------------
echristo wrote:
> Sounds like an assert.
we do assert on that, will update the doc here.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:534
+    }
+    if (!canMoveTo(*I, *TargetRegion.BranchBlock, false)) {
+      DEBUG(dbgs() << "Instruction " << *I
----------------
echristo wrote:
> else if? Otherwise what happens?
No else if.  We check conditions to move up and down separately and verify that only one direction is valid.


https://reviews.llvm.org/D28249