[PATCH] D28249: Improve scheduling with branch coalescing

[Lei Huang via Phabricator via llvm-commits]

lei marked 8 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:
> > > 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;
> >     }
> >   }
> > 
> > ```
> > 
> 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.
see code snippet above.  


https://reviews.llvm.org/D28249