[PATCH] D28249: Improve scheduling with branch coalescing
Nemanja Ivanovic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 9 17:09:57 PST 2017
nemanjai added a comment.
These inline comments are based on a first-pass reading of the patch. As such, they refer to issues that are rather local in scope. This is a large patch so I'll set aside some more time to fully understand it in its entirety in the next review cycle.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:217
+ MRI = &MF.getRegInfo();
+ NumBlocksCoalesced = 0;
+ NumPHINotMoved = 0;
----------------
We want to reset the stats for every function? I would have assumed we're interested in collecting this for the entire module.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:227
+/// BranchTargetBlock and the FallThroughBlock are recorded in the specified
+/// Candidate. Note that Cand can be modified even if this method fails.
+///
----------------
I am personally not a fan of functions that modify an output parameter and then fail. Presumably this makes no difference now, but if we end up extending this pass in the future to have some backtracking/requeuing capabilities, it would be nice if the object wasn't modified/invalidated in some way.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:336
+ DEBUG(dbgs() << "Op1: " << Op1 << "\n"
+ << "Op2:" << Op2 << "\n");
+
----------------
Use same debug format for both (i.e. space after the colon).
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:344
+ if (TargetRegisterInfo::isVirtualRegister(Op1.getReg()) &&
+ TargetRegisterInfo::isVirtualRegister(Op2.getReg())) {
+ MachineInstr *Op1Def = MRI->getVRegDef(Op1.getReg());
----------------
Won't this fail (as in assert) if `Op2.isReg() != true`?
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:356
+ TargetRegisterInfo::isPhysicalRegister(Op2.getReg())) {
+ assert(0 && "Physical register!!");
+ }
----------------
Maybe a comment as to why we fail here if we get physical registers. Also, if I follow the control flow here correctly, we will return `true` here if one is a physical register and the other is a virtual register. Is that what we want? I imagine not.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:378
+ MachineBasicBlock::instr_iterator I = MBB.instr_begin(), E = MBB.instr_end();
+ while (I != E && !I->isPHI())
+ ++I;
----------------
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.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:393
+ MachineBasicBlock *ToMBB) {
+ MachineBasicBlock::iterator InsertLoc = getFirstPHI(*ToMBB);
+
----------------
Can you please state why something like `MachineBasicBlock::SkipPHIsLabelsAndDebug()` cannot be used instead of this function (thereby eliminating the need for that function altogether)?
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:409
+ }
+ ToMBB->splice(InsertLoc, FromMBB, &PHIInst);
+ }
----------------
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.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:415
+/// Determine if the specified instruction can be moved to the specified machine
+/// basic block. This method will determine if the specified instruction can be
+/// moved to the specified block. If MoveToBeginning is set to true, it checks
----------------
The second sentence is redundant.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:419
+/// following any PHI instructions). In this case, it checks whether the
+/// register(s) defined in this instruction are used in this block or in any PHI
+/// instructions at the beginning of the specified block.
----------------
'... used in this block ...' means '... used in original block ...'?
Overall, I think this comment is kind of hard to follow. I think it would suffice to say when an instruction can move to the begin/end location. Something along the lines of:
An instruction MI can move from MBB Source to MBB Target under the following conditions:
- There are no PHI's in Target that use what MI defines
- No instructions in Source use what MI defines (unless Target is a dominator of Source)
- No instructions in Source define what MI uses (unless Source is a dominator of Target)
- If any instructions in Target define what MI uses, the MI can only move to the end of Target
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:442
+ for (auto &Def : MI.defs()) { // Looking at Def
+ if (Def.isReg()) // could it be anything else?
+ for (auto &Use : MRI->use_instructions(Def.getReg())) {
----------------
I don't understand the purpose of the comment.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:466
+
+ DEBUG(dbgs() << " Safe to move\n");
+ return true;
----------------
I don't think this function suffices to determine whether an instruction can move to a block.
Example MBB's:
```
BB#0:
...
; some non-PHI def of %vreg2
; some non-PHI def of %vreg3
%vreg10 = ADD %vreg2, %vreg3 ; MI
...
BB#1: ; MBB1
%vreg5 = PHI %vreg10, <BB#0>, %vreg4, <BB#2>
...
```
The call:
` canMoveTo(MI, MBB1, false)`
Would return true, but it is not safe to do so.
I think this patch uses this query in very limited circumstances, so it isn't necessarily required for this function to determine whether it is safe to move an arbitrary instruction to an arbitrary block. But if this is the case, we should add asserts to eliminate incorrect uses of the function (in the future).
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:507
+
+ assert(To.FallThroughBlock->empty() && From.FallThroughBlock->empty() &&
+ "Expecting fall-through blocks to be empty");
----------------
Why are all of these asserts rather than early exits returning false? On builds that compile asserts out, I imagine we could end up doing the wrong thing.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:510
+
+ DEBUG(dbgs() << "Entering " << __PRETTY_FUNCTION__ << "\n");
+
----------------
I don't think there's anything to be gained from printing the name of the function emitting the message in the debug output. Please remove this.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:554
+///
+/// Merge blocks from From into blocks from To and remove From blocks from
+/// function.
----------------
The use of 'From', 'from', 'To' and 'to' makes the comment confusing. Please consider renaming the parameters or otherwise clarifying this comment. Perhaps something like:
`Merge the CFG triangle \p From into the preceding CFG triangle \p To.`
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:608
+
+ assert(To.FallThroughBlock->isSuccessor(From.BranchBlock) &&
+ "Expecting From.BranchBlock to be a successor of To.FallThroughBlock");
----------------
Are these two asserts enough or does `From.BranchBlock` need to [immediately] dominate both `To.BranchTargetBlock` and `To.FallThroughBlock`?
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:620
+
+ for (MachineBasicBlock::iterator MI = From.BranchBlock->getFirstNonPHI();
+ MI != End;) {
----------------
You've already moved all the PHI's from `From.BranchBlock`. Can't this just be a loop over `From.BranchBlock->instrs()`? Or it may need to be a loop from `rbegin()` to `rend()`.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:626
+ ++MI;
+ if (From.MustMoveDown) {
+ assert(!From.MustMoveUp &&
----------------
This is loop-invariant. I think it'd be cleaner to move the assert out of the loop and call `splice()` on the result of a ternary operator. Maybe even invert the `if` and the loop.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:655
+ // Move any PHIs down to the branch-taken block
+ // It is not necessary to merge the fall-through blocks because they are
+ // empty!
----------------
Where is the assert to ensure this?
https://reviews.llvm.org/D28249
More information about the llvm-commits
mailing list