[FastISel][X86] Constrain register operand class in foldOperandImpl

[loladiro]

Summary

instead of cleaning up after. This is made possible by a previous
commit (D10174) that threaded InsertPt through foldOperandImpl.

[ributzka]

I think this should be done inside 'foldMemoryOperandImpl'. We shouldn't have to cleanup afterwards.

'constrainRegClass' may actually fail, so you need to check the result of this method if you use it .

lib/Target/X86/X86FastISel.cpp

3348

I don't think this is correct. The index "i" must line up with the MO index in the instruction and not with AddrOps.

3350

MO.isReg() is shorter :)

3353

If you use TargetRegisterInfo::isVirtualRegister instead, you can even skip the MO.isReg() check.

3355

This is not guaranteed to succeed - you need to check the result.

See FastISel::constrainOperandRegClass for more details.

[loladiro]

I looked at implementing this in foldMemoryOperandImpl, but it seems like right now it can only return one instruction and does not insert it, which doesn't work if we want to insert the COPY for constraining the register class.

I did see a FIXME that says

// FIXME: change foldMemoryOperandImpl semantics to also insert NewMI.

so maybe I should just try that?

[loladiro]

Ok, have a look at this and let me know what you think. It threads through the InsertPt and then sets the correct register class in foldMemoryOperandImpl. If this approach generally looks fine, I'll split out threading through the InsertPt into a separate commit (since it touches other backends).

[loladiro]

@pete looks like you fixed this one as well (though this time in rL236644). Maybe I'll just wait for somebody else to fix it next time ;). Would you mind taking a look here and seeing if this is still interesting?

[pete]

In D6262#181993, @loladiro wrote:

@pete looks like you fixed this one as well (though this time in rL236644). Maybe I'll just wait for somebody else to fix it next time ;). Would you mind taking a look here and seeing if this is still interesting?

Yep, looks like my fix handled this one too. I don't think this patch will handle more cases than mine. If you have a strong preference for your solution over mine, then i'm happy to see the code changed though. There's no perfect way to fix this bug so i'm happy to see the best possible solution committed.

[loladiro]

Well, as you can see the first version of this patch was actually exactly the same as yours and @ributzka asked me to refactor it. It's been a while since I looked at this code, but it seems like I can factor out adding the InsertPt, which is the bulk of this change as an NFC commit and then only have the actual bugfix here. Does that sounds good?

[pete]

Yeah, sounds good to me. Thanks!

[loladiro]

Split out the NFC changes as D10174. This is what remains.

[loladiro]

Ok, the other changes have gone in. @pete does this look ok to you?

[pete]

Sorry, Phab is acting up, so will use email.

This was in the original, but as your moving the code anyway, can you invert this to remove indentation, i.e.,

+ if (TargetRegisterInfo::isVirtualRegister(Op)) {
…
}
return Op

to

+ if (!TargetRegisterInfo::isVirtualRegister(Op))

return Op;

…

Similarly, can you make this a foreach loop:

for (unsigned i = 0; i != NumAddrOps; ++i) {

to

for (MachineOperand MO : MOs) {

Thanks for doing all this!

Cheers,
Pete

[loladiro]

The i in the for loop is actually required as it's passed into constrainOperandRegClass. For the other one, the success case of constrainRegClass, does also lead to the second return statement, so I think it's cleaner this way than

unsigned MachineRegisterInfo::constrainOperandRegClass(
    MachineFunction &MF, MachineBasicBlock::iterator InsertPt, DebugLoc DL,
    const MCInstrDesc &II, unsigned Op, unsigned OpNum,
    const TargetInstrInfo &TII, const TargetRegisterInfo &TRI) {
  if (!TargetRegisterInfo::isVirtualRegister(Op))
    return Op;
  const TargetRegisterClass *RegClass = TII.getRegClass(II, OpNum, &TRI, MF);
  if (constrainRegClass(Op, RegClass))
    return Op;
  // If it's not legal to COPY between the register classes, something
  // has gone very wrong before we got here.
  unsigned NewOp = createVirtualRegister(RegClass);
  BuildMI(*InsertPt->getParent(), InsertPt, DL, TII.get(TargetOpcode::COPY),
          NewOp)
      .addReg(Op);
  return NewOp;
}

but I have no strong opinion.

include/llvm/CodeGen/MachineRegisterInfo.h

                                                const TargetRegisterClass *RC,
                                                unsigned MinNumRegs = 0);
 
+  /// \brief Try to constrain Op so that it is usable by argument OpNum of the
+  /// provided MCInstrDesc. If this fails, create a new virtual register in the
+  /// correct class and COPY the value there.
+  unsigned constrainOperandRegClass(MachineFunction &MF,
+                                    MachineBasicBlock::iterator InsertPt,
+                                    DebugLoc DL, const MCInstrDesc &II,
+                                    unsigned Op, unsigned OpNum,
+                                    const TargetInstrInfo &TII,
+                                    const TargetRegisterInfo &TRI);
+
   /// recomputeRegClass - Try to find a legal super-class of Reg's register
   /// class that still satisfies the constraints from the instructions using
   /// Reg.  Returns true if Reg was upgraded.

lib/CodeGen/MachineRegisterInfo.cpp

   return NewRC;
 }
 
+unsigned MachineRegisterInfo::constrainOperandRegClass(
+    MachineFunction &MF, MachineBasicBlock::iterator InsertPt, DebugLoc DL,
+    const MCInstrDesc &II, unsigned Op, unsigned OpNum,
+    const TargetInstrInfo &TII, const TargetRegisterInfo &TRI) {
+  if (TargetRegisterInfo::isVirtualRegister(Op)) {
+    const TargetRegisterClass *RegClass = TII.getRegClass(II, OpNum, &TRI, MF);
+    if (!constrainRegClass(Op, RegClass)) {
+      // If it's not legal to COPY between the register classes, something
+      // has gone very wrong before we got here.
+      unsigned NewOp = createVirtualRegister(RegClass);
+      BuildMI(*InsertPt->getParent(), InsertPt, DL, TII.get(TargetOpcode::COPY),
+              NewOp)
+          .addReg(Op);
+      return NewOp;
+    }
+  }
+  return Op;
+}
+
 bool
 MachineRegisterInfo::recomputeRegClass(unsigned Reg) {
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();

lib/CodeGen/SelectionDAG/FastISel.cpp

 
 unsigned FastISel::constrainOperandRegClass(const MCInstrDesc &II, unsigned Op,
                                             unsigned OpNum) {
-  if (TargetRegisterInfo::isVirtualRegister(Op)) {
-    const TargetRegisterClass *RegClass =
-        TII.getRegClass(II, OpNum, &TRI, *FuncInfo.MF);
-    if (!MRI.constrainRegClass(Op, RegClass)) {
-      // If it's not legal to COPY between the register classes, something
-      // has gone very wrong before we got here.
-      unsigned NewOp = createResultReg(RegClass);
-      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
-              TII.get(TargetOpcode::COPY), NewOp).addReg(Op);
-      return NewOp;
-    }
-  }
-  return Op;
+  return MRI.constrainOperandRegClass(*FuncInfo.MF, FuncInfo.InsertPt, DbgLoc,
+                                      II, Op, OpNum, TII, TRI);
 }
 
 unsigned FastISel::fastEmitInst_(unsigned MachineInstOpcode,

lib/Target/X86/X86FastISel.cpp

   if (!Result)
     return false;
 
-  // The index register could be in the wrong register class.  Unfortunately,
-  // foldMemoryOperandImpl could have commuted the instruction so its not enough
-  // to just look at OpNo + the offset to the index reg.  We actually need to
-  // scan the instruction to find the index reg and see if its the correct reg
-  // class.
-  unsigned OperandNo = 0;
-  for (MachineInstr::mop_iterator I = Result->operands_begin(),
-       E = Result->operands_end(); I != E; ++I, ++OperandNo) {
-    MachineOperand &MO = *I;
-    if (!MO.isReg() || MO.isDef() || MO.getReg() != AM.IndexReg)
-      continue;
-    // Found the index reg, now try to rewrite it.
-    unsigned IndexReg = constrainOperandRegClass(Result->getDesc(),
-                                                 MO.getReg(), OperandNo);
-    if (IndexReg == MO.getReg())
-      continue;
-    MO.setReg(IndexReg);
-  }
-
   Result->addMemOperand(*FuncInfo.MF, createMachineMemOperandFor(LI));
   MI->eraseFromParent();
   return true;

lib/Target/X86/X86InstrInfo.cpp

   return false;
 }
 
-void addOperands(MachineInstrBuilder &MIB, ArrayRef<MachineOperand> MOs) {
+static void addOperands(MachineFunction &MF, MachineInstrBuilder &MIB,
+                        unsigned Offset, const MCInstrDesc &Desc,
+                        ArrayRef<MachineOperand> MOs,
+                        MachineBasicBlock::iterator InsertPt, MachineInstr *MI,
+                        const TargetInstrInfo &TII) {
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+  const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
+
   unsigned NumAddrOps = MOs.size();
-  for (unsigned i = 0; i != NumAddrOps; ++i)
-    MIB.addOperand(MOs[i]);
+  for (unsigned i = 0; i != NumAddrOps; ++i) {
+    MachineOperand MO = MOs[i];
+    if (MO.isReg()) {
+      MO.setReg(MRI.constrainOperandRegClass(MF, InsertPt, MI->getDebugLoc(),
+                                             Desc, MO.getReg(), i + Offset, TII,
+                                             TRI));
+    }
+    MIB.addOperand(MO);
+  }
   if (NumAddrOps < 4) // FrameIndex only
     addOffset(MIB, 0);
 }

                                      const TargetInstrInfo &TII) {
   // Create the base instruction with the memory operand as the first part.
   // Omit the implicit operands, something BuildMI can't do.
-  MachineInstr *NewMI = MF.CreateMachineInstr(TII.get(Opcode),
-                                              MI->getDebugLoc(), true);
+  const MCInstrDesc &MCID = TII.get(Opcode);
+  MachineInstr *NewMI = MF.CreateMachineInstr(MCID, MI->getDebugLoc(), true);
   MachineInstrBuilder MIB(MF, NewMI);
-  addOperands(MIB, MOs);
+  addOperands(MF, MIB, 0, MCID, MOs, InsertPt, MI, TII);
 
   // Loop over the rest of the ri operands, converting them over.
   unsigned NumOps = MI->getDesc().getNumOperands()-2;

                               MachineBasicBlock::iterator InsertPt,
                               MachineInstr *MI, const TargetInstrInfo &TII) {
   // Omit the implicit operands, something BuildMI can't do.
-  MachineInstr *NewMI = MF.CreateMachineInstr(TII.get(Opcode),
-                                              MI->getDebugLoc(), true);
+  const MCInstrDesc &MCID = TII.get(Opcode);
+  MachineInstr *NewMI = MF.CreateMachineInstr(MCID, MI->getDebugLoc(), true);
   MachineInstrBuilder MIB(MF, NewMI);
 
   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
     MachineOperand &MO = MI->getOperand(i);
     if (i == OpNo) {
       assert(MO.isReg() && "Expected to fold into reg operand!");
-      addOperands(MIB, MOs);
+      addOperands(MF, MIB, OpNo, MCID, MOs, InsertPt, MI, TII);
     } else {
       MIB.addOperand(MO);
     }

                                 ArrayRef<MachineOperand> MOs,
                                 MachineBasicBlock::iterator InsertPt,
                                 MachineInstr *MI) {
-  MachineInstrBuilder MIB = BuildMI(*InsertPt->getParent(), InsertPt,
-                                    MI->getDebugLoc(), TII.get(Opcode));
-  addOperands(MIB, MOs);
+  const MCInstrDesc &MCID = TII.get(Opcode);
+  MachineInstrBuilder MIB =
+      BuildMI(*InsertPt->getParent(), InsertPt, MI->getDebugLoc(), MCID);
+  addOperands(*InsertPt->getParent()->getParent(), MIB, 0, MCID, MOs, InsertPt,
+              MI, TII);
   return MIB.addImm(0);
 }