[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei marked 12 inline comments as done.
lei 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());
----------------
nemanjai wrote:
> 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;
> }
> ```
Minor change so that we are doing early exit when operands within the operand lists are detected to be diff.

```
 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");
      continue;
    }

    // If the operands are not identical, but are registers, check to see if the
    // definition of the register produces the same value. If they produce the
    // same value, consider them to be identical.
    if (Op1.isReg() && Op2.isReg() &&
        TargetRegisterInfo::isVirtualRegister(Op1.getReg()) &&
        TargetRegisterInfo::isVirtualRegister(Op2.getReg())) {
      MachineInstr *Op1Def = MRI->getVRegDef(Op1.getReg());
      MachineInstr *Op2Def = MRI->getVRegDef(Op2.getReg());
      if (TII->produceSameValue(*Op1Def, *Op2Def, MRI)) {
        DEBUG(dbgs() << "Op1Def: " << *Op1Def << " and " << *Op2Def
                     << " produce the same value!\n");
      } else {
        DEBUG(dbgs() << "Operands produce different values\n");
        return false;
      }
    } else {
      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!!");
+        }
----------------
nemanjai wrote:
> 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).
changes in previous comment will address this issue as well.


================
Comment at: lib/CodeGen/BranchCoalescing.cpp:466
+
+  DEBUG(dbgs() << "  Safe to move\n");
+  return true;
----------------
nemanjai wrote:
> 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.
We always return false when an instruction isn't safe to remove.  This function just does a check to see if it's valid to move an instruction to the BB at the specified location. If won't work if it asserts when it can't be moved.


https://reviews.llvm.org/D28249