[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());
----------------
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;
> }
> ```
Sorry, the two `return true;` lines should be `continue;` so that you keep checking the remainder of the lists rather than returning true if the first pair are identical.


https://reviews.llvm.org/D28249