[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei marked 6 inline comments as done.
lei added inline comments.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:378
+  MachineBasicBlock::instr_iterator I = MBB.instr_begin(), E = MBB.instr_end();
+  while (I != E && !I->isPHI())
+    ++I;
----------------
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.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:386
+/// instructions in From into To and update them to refer to the new block. They
+/// are placed at the beginning of To, after any existing PHI instructions.
+///
----------------
nemanjai wrote:
> It seems below like you're inserting them **before** any existing PHI instructions.
Yes. They should be inserted before existing PHIs.  Will update the doc.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:393
+                                         MachineBasicBlock *ToMBB) {
+  MachineBasicBlock::iterator InsertLoc = getFirstPHI(*ToMBB);
+
----------------
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.


https://reviews.llvm.org/D28249