[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei marked 3 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;
----------------
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()


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:409
+    }
+    ToMBB->splice(InsertLoc, FromMBB, &PHIInst);
+  }
----------------
nemanjai wrote:
> You should have a range of iterators that represent the PHI's in the `From` block. Why not use the range-version of `splice()` after this loop?
> If it is to avoid the empty range check on the PHI's in the `From` block, maybe just a comment to that end.
will update to use range version of splice()


https://reviews.llvm.org/D28249