[PATCH] D28249: Improve scheduling with branch coalescing

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:344
+        if (TargetRegisterInfo::isVirtualRegister(Op1.getReg()) &&
+            TargetRegisterInfo::isVirtualRegister(Op2.getReg())) {
+          MachineInstr *Op1Def = MRI->getVRegDef(Op1.getReg());
----------------
lei wrote:
> 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.  
Oh OK. I missed the early exit check. I think the layout of this function is a little confusing so I can see why I missed it.
It seems that the conditions under which we return true are (any of):
- The lists are pair-wise identical
- For any pairs that aren't identical, they're both registers that produce the same value

Why not such a simple layout to the function? For example, your check for same type is repeated in the call to `MachineOperand::isIdenticalTo()`.
It seems to me like a layout such as this for the loop encapsulates what this function should do:
```
for (unsigned i = 0; i < OpList1.size(); ++i) {
  const MachineOperand &Op1 = OpList1[i];
  const MachineOperand &Op2 = OpList2[i];
  DEBUG(dbgs() << "Op1: " << Op1 << "\n"
               << "Op2: " << Op2 << "\n");

  if (Op1.isIdenticalTo(Op2)) {
    DEBUG(dbgs() << "Op1 and Op2 are identical!\n");
    return true;
  }

  if (Op1.isReg() && Op2.isReg() &&
      TargetRegisterInfo::isVirtualRegister(Op1.getReg()) &&
      TargetRegisterInfo::isVirtualRegister(Op2.getReg())) {
    if (TII->produceSameValue(*Op1Def, *Op2Def, MRI)) {
      DEBUG(dbgs() << "Operands produce the same value.\n");
      return true;
    }
  }
  DEBUG(dbgs() << "The operands are not provably identical.\n");
  return false;
}
```


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:356
+                   TargetRegisterInfo::isPhysicalRegister(Op2.getReg())) {
+          assert(0 && "Physical register!!");
+        }
----------------
lei wrote:
> 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.
I'm afraid this won't suffice. If it were to somehow happen that (at least) one is a physical register, you'll return true here - regardless of whether it's the same register (not that it really matters whether it's the same register or 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;
----------------
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.


================
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.
+///
----------------
It seems below like you're inserting them **before** any existing PHI instructions.


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


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:466
+
+  DEBUG(dbgs() << "  Safe to move\n");
+  return true;
----------------
lei wrote:
> 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.
Yes, but please add asserts to ensure this function isn't used incorrectly in the future. It is dangerous for a function called `canMoveTo` to return true when an instruction can't safely move to a basic block.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:608
+
+  assert(To.FallThroughBlock->isSuccessor(From.BranchBlock) &&
+         "Expecting From.BranchBlock to be a successor of To.FallThroughBlock");
----------------
lei wrote:
> 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.
The two are not mutually exclusive. Also, this doesn't answer whether `From.BranchBlock` needs to dominate `To.FallThroughBlock`.


================
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!
----------------
lei wrote:
> nemanjai wrote:
> > Where is the assert to ensure this?
> In analyzeBranch(), line 297, we do an early exit if the FallThroughBlock contain code.
Sure. Now there is. I'd still prefer an assert. If someone in the future feels that `analyzeBranch()` doesn't need that early exit any longer, they'll need to ensure they account for this assert and why it's in this function.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:729
+
+      if (MergedCandidates) {
+        DEBUG(dbgs() << "Function after merging: "; MF.dump(); dbgs() << "\n");
----------------
Setting a bool to true and then using it in an if statement without any statements that could modify it in between is redundant.
Besides, I think it's overly verbose to dump the entire function after every merge - I'd imagine that a dump after all the merging is done would suffice.


https://reviews.llvm.org/D28249