[PATCH] D28249: Improve scheduling with branch coalescing
Lei Huang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 22 09:58:33 PST 2017
lei marked 12 inline comments as done.
lei added inline comments.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:217
+ MRI = &MF.getRegInfo();
+ NumBlocksCoalesced = 0;
+ NumPHINotMoved = 0;
----------------
nemanjai wrote:
> We want to reset the stats for every function? I would have assumed we're interested in collecting this for the entire module.
Initialization removed.
================
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.
+///
----------------
nemanjai wrote:
> 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.
agreed.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:336
+ DEBUG(dbgs() << "Op1: " << Op1 << "\n"
+ << "Op2:" << Op2 << "\n");
+
----------------
nemanjai wrote:
> Use same debug format for both (i.e. space after the colon).
fixed
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:344
+ if (TargetRegisterInfo::isVirtualRegister(Op1.getReg()) &&
+ TargetRegisterInfo::isVirtualRegister(Op2.getReg())) {
+ MachineInstr *Op1Def = MRI->getVRegDef(Op1.getReg());
----------------
nemanjai wrote:
> Won't this fail (as in assert) if `Op2.isReg() != true`?
No, since we check that Op2 and Op1 are the same type on line 330. We do an early exit if they are not the same type. getType() returns an enum MachineOperandType, isReg() basically checks that the MachineOperand's type (MachineOperandType) is of the specific enum MO_Register.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:356
+ TargetRegisterInfo::isPhysicalRegister(Op2.getReg())) {
+ assert(0 && "Physical register!!");
+ }
----------------
nemanjai wrote:
> 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.
It was just put in to see if we trip on it. I have removed this.
================
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:
> 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.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:393
+ MachineBasicBlock *ToMBB) {
+ MachineBasicBlock::iterator InsertLoc = getFirstPHI(*ToMBB);
+
----------------
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.
================
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
----------------
nemanjai wrote:
> The second sentence is redundant.
agreed
================
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.
----------------
nemanjai wrote:
> '... 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
Arguments renamed and doc updated to be more clear.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:466
+
+ DEBUG(dbgs() << " Safe to move\n");
+ return true;
----------------
nemanjai wrote:
> 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).
You are right, this function alone can not be used to determine whether an instruction can move to a random block. This function is a helper function used by canMerge() to determine whether 2 candidates can be merged.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:507
+
+ assert(To.FallThroughBlock->empty() && From.FallThroughBlock->empty() &&
+ "Expecting fall-through blocks to be empty");
----------------
nemanjai wrote:
> 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.
This seem to be the case for other classes like MachineOperand. If assert is off it could end up doing the wrong thing....
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:510
+
+ DEBUG(dbgs() << "Entering " << __PRETTY_FUNCTION__ << "\n");
+
----------------
nemanjai wrote:
> 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.
okay
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:554
+///
+/// Merge blocks from From into blocks from To and remove From blocks from
+/// function.
----------------
nemanjai wrote:
> 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.`
Renamed "From/To" to "SourceRegion/TargetRegion". I believe this is now less confusing ...
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:608
+
+ assert(To.FallThroughBlock->isSuccessor(From.BranchBlock) &&
+ "Expecting From.BranchBlock to be a successor of To.FallThroughBlock");
----------------
nemanjai wrote:
> Are these two asserts enough or does `From.BranchBlock` need to [immediately] dominate both `To.BranchTargetBlock` and `To.FallThroughBlock`?
From.BranchBlock MUST equal To.BranchTargetBlock, not dominate.
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:620
+
+ for (MachineBasicBlock::iterator MI = From.BranchBlock->getFirstNonPHI();
+ MI != End;) {
----------------
nemanjai wrote:
> 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()`.
agreed
================
Comment at: lib/CodeGen/BranchCoalescing.cpp:626
+ ++MI;
+ if (From.MustMoveDown) {
+ assert(!From.MustMoveUp &&
----------------
nemanjai wrote:
> 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.
assert moved to top of function
================
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!
----------------
nemanjai wrote:
> Where is the assert to ensure this?
In analyzeBranch(), line 297, we do an early exit if the FallThroughBlock contain code.
https://reviews.llvm.org/D28249
More information about the llvm-commits
mailing list