[MachineVerifier] Add check that renamable operands aren't reserved registers.

[gberry]

Summary

Also clean up a few places that were modifying code after register
allocation to set the renamable bit correctly to avoid failing this validation.

[qcolombet]

lib/CodeGen/MachineVerifier.cpp

1111

I know we already touched that in the other thread, but are we definitely giving up on all optimizations as soon as a MachineOperand has been assigned a reserved register?

I guess we could say that given the liveness of reserved registers is not necessarily properly modeled, when such register is assigned to an operand, it won't be safe later on to change it, so that's expected.

lib/CodeGen/TargetInstrInfo.cpp

182

Looking at this snippet only, one could find it strange to have virtual registers not being renamable but when you look at the whole picture, we see these variables are not used for virtual registers thus we really don't care what we put here.

Could you add a comment saying that this will be used only for PhysReg?
(IIRC isRenamable does not work with VReg anyway.)

[gberry]

Add comments to address feedback from Quentin

[gberry]

lib/CodeGen/MachineVerifier.cpp

1111

Yes, I agree with your answer to your question here :)

[MatzeB]

I was contemplating whether we need a check that the register assigned on a renamable operand is a physreg. Of course we cannot even write a verifier for that as using MO::isRenamable() will immediately run into an assertion failure when that is not the case (which is good!).

I think the way to solve this would be to move the machine verifier logic around to a MachineOperand::verify() function. This would be nice in that it can access machine operand internals that way. On the other hand this would probably need a bunch of refactoring to make it play seamless with the machine verifier reporting infrastructure. So I don't expect this to be solved here (or by you), just writing my thoughts down.

LGTM, would be nice if you could split the fixes and the verifier changes into separate commits when pushing this.

include/llvm/CodeGen/MachineInstr.h

public:
   }
 
   /// Clears kill flags on all operands.
   void clearKillInfo();
 
   /// Replace all occurrences of FromReg with ToReg:SubIdx,
   /// properly composing subreg indices where necessary.
   void substituteRegister(unsigned FromReg, unsigned ToReg, unsigned SubIdx,
-                          const TargetRegisterInfo &RegInfo);
+                          const TargetRegisterInfo &RegInfo,
+                          bool ClearIsRenamable = false);
 
   /// We have determined MI kills a register. Look for the
   /// operand that uses it and mark it as IsKill. If AddIfNotFound is true,
   /// add a implicit operand if it's not found. Returns true if the operand
   /// exists / is added.
   bool addRegisterKilled(unsigned IncomingReg,
                          const TargetRegisterInfo *RegInfo,
                          bool AddIfNotFound = false);

lib/CodeGen/MachineInstr.cpp

 ///
 void MachineInstr::clearKillInfo() {
   for (MachineOperand &MO : operands()) {
     if (MO.isReg() && MO.isUse())
       MO.setIsKill(false);
   }
 }
 
-void MachineInstr::substituteRegister(unsigned FromReg,
-                                      unsigned ToReg,
+void MachineInstr::substituteRegister(unsigned FromReg, unsigned ToReg,
                                       unsigned SubIdx,
-                                      const TargetRegisterInfo &RegInfo) {
+                                      const TargetRegisterInfo &RegInfo,
+                                      bool ClearIsRenamable) {
   if (TargetRegisterInfo::isPhysicalRegister(ToReg)) {
     if (SubIdx)
       ToReg = RegInfo.getSubReg(ToReg, SubIdx);
     for (MachineOperand &MO : operands()) {
       if (!MO.isReg() || MO.getReg() != FromReg)
         continue;
       MO.substPhysReg(ToReg, RegInfo);
+      if (ClearIsRenamable)
+        MO.setIsRenamable(false);
     }
   } else {
+    assert(!ClearIsRenamable && "IsRenamable invalid for virtual registers");
     for (MachineOperand &MO : operands()) {
       if (!MO.isReg() || MO.getReg() != FromReg)
         continue;
       MO.substVirtReg(ToReg, SubIdx, RegInfo);
     }
   }
 }
 

lib/CodeGen/MachineVerifier.cpp

if (TargetRegisterInfo::isPhysicalRegister(Reg)) {
               TII->getRegClass(MCID, MONum, TRI, *MF)) {
           if (!DRC->contains(Reg)) {
             report("Illegal physical register for instruction", MO, MONum);
             errs() << printReg(Reg, TRI) << " is not a "
                    << TRI->getRegClassName(DRC) << " register.\n";
           }
         }
       }
-      if (MO->isRenamable() &&
-          ((MO->isDef() && MI->hasExtraDefRegAllocReq()) ||
-           (MO->isUse() && MI->hasExtraSrcRegAllocReq()))) {
+      if (MO->isRenamable()) {
+        if ((MO->isDef() && MI->hasExtraDefRegAllocReq()) ||
+            (MO->isUse() && MI->hasExtraSrcRegAllocReq()))
           report("Illegal isRenamable setting for opcode with extra regalloc "
                  "requirements",
                  MO, MONum);
+        if (MRI->isReserved(Reg))
+          report("isRenamable set on reserved register", MO, MONum);
         return;
       }
     } else {
       // Virtual register.
       const TargetRegisterClass *RC = MRI->getRegClassOrNull(Reg);
       if (!RC) {
         // This is a generic virtual register.
 

lib/CodeGen/TargetInstrInfo.cpp

MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
   unsigned SubReg1 = MI.getOperand(Idx1).getSubReg();
   unsigned SubReg2 = MI.getOperand(Idx2).getSubReg();
   bool Reg1IsKill = MI.getOperand(Idx1).isKill();
   bool Reg2IsKill = MI.getOperand(Idx2).isKill();
   bool Reg1IsUndef = MI.getOperand(Idx1).isUndef();
   bool Reg2IsUndef = MI.getOperand(Idx2).isUndef();
   bool Reg1IsInternal = MI.getOperand(Idx1).isInternalRead();
   bool Reg2IsInternal = MI.getOperand(Idx2).isInternalRead();
+  bool Reg1IsRenamable = TargetRegisterInfo::isPhysicalRegister(Reg1)
+                             ? MI.getOperand(Idx1).isRenamable()
+                             : false;
+  bool Reg2IsRenamable = TargetRegisterInfo::isPhysicalRegister(Reg2)
+                             ? MI.getOperand(Idx2).isRenamable()
+                             : false;
   // If destination is tied to either of the commuted source register, then
   // it must be updated.
   if (HasDef && Reg0 == Reg1 &&
       MI.getDesc().getOperandConstraint(Idx1, MCOI::TIED_TO) == 0) {
     Reg2IsKill = false;
     Reg0 = Reg2;
     SubReg0 = SubReg2;
   } else if (HasDef && Reg0 == Reg2 &&
MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
   CommutedMI->getOperand(Idx2).setSubReg(SubReg1);
   CommutedMI->getOperand(Idx1).setSubReg(SubReg2);
   CommutedMI->getOperand(Idx2).setIsKill(Reg1IsKill);
   CommutedMI->getOperand(Idx1).setIsKill(Reg2IsKill);
   CommutedMI->getOperand(Idx2).setIsUndef(Reg1IsUndef);
   CommutedMI->getOperand(Idx1).setIsUndef(Reg2IsUndef);
   CommutedMI->getOperand(Idx2).setIsInternalRead(Reg1IsInternal);
   CommutedMI->getOperand(Idx1).setIsInternalRead(Reg2IsInternal);
+  if (TargetRegisterInfo::isPhysicalRegister(Reg1))
+    CommutedMI->getOperand(Idx2).setIsRenamable(Reg1IsRenamable);
+  if (TargetRegisterInfo::isPhysicalRegister(Reg2))
+    CommutedMI->getOperand(Idx1).setIsRenamable(Reg2IsRenamable);
   return CommutedMI;
 }
 
 MachineInstr *TargetInstrInfo::commuteInstruction(MachineInstr &MI, bool NewMI,
                                                   unsigned OpIdx1,
                                                   unsigned OpIdx2) const {
   // If OpIdx1 or OpIdx2 is not specified, then this method is free to choose
   // any commutable operand, which is done in findCommutedOpIndices() method

lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

if (CopyFromExecInst == E) {
       if (PrepareExecInst == E)
         continue;
       // Fold exec = COPY (S_AND_B64 reg, exec) -> exec = S_AND_B64 reg, exec
       if (CopyToExecInst->getOperand(1).isKill() &&
           isLogicalOpOnExec(*PrepareExecInst) == CopyToExec) {
         DEBUG(dbgs() << "Fold exec copy: " << *PrepareExecInst);
 
         PrepareExecInst->getOperand(0).setReg(AMDGPU::EXEC);
+        PrepareExecInst->getOperand(0).setIsRenamable(false);
 
         DEBUG(dbgs() << "into: " << *PrepareExecInst << '\n');
 
         CopyToExecInst->eraseFromParent();
       }
 
       continue;
     }
BuildMI(MBB, InsPt, DL, TII->get(getSaveExecOp(SaveExecInst->getOpcode())),
             CopyFromExec)
       .addReg(OtherOp->getReg());
     SaveExecInst->eraseFromParent();
 
     CopyToExecInst->eraseFromParent();
 
     for (MachineInstr *OtherInst : OtherUseInsts) {
       OtherInst->substituteRegister(CopyToExec, AMDGPU::EXEC,
-                                    AMDGPU::NoSubRegister, *TRI);
+                                    AMDGPU::NoSubRegister, *TRI,
+                                    /*ClearIsRenamable=*/true);
     }
   }
 
   return true;
 
 }

lib/Target/X86/X86FloatingPoint.cpp

void FPS::handleCompareFP(MachineBasicBlock::iterator &I) {
   bool KillsOp1 = MI.killsRegister(X86::FP0 + Op1);
 
   // Make sure the first operand is on the top of stack, the other one can be
   // anywhere.
   moveToTop(Op0, I);
 
   // Change from the pseudo instruction to the concrete instruction.
   MI.getOperand(0).setReg(getSTReg(Op1));
+  MI.getOperand(0).setIsRenamable(false);
   MI.RemoveOperand(1);
   MI.setDesc(TII->get(getConcreteOpcode(MI.getOpcode())));
 
   // If any of the operands are killed by this instruction, free them.
   if (KillsOp0) freeStackSlotAfter(I, Op0);
   if (KillsOp1 && Op0 != Op1) freeStackSlotAfter(I, Op1);
 }
 
void FPS::handleCondMovFP(MachineBasicBlock::iterator &I) {
   // The first operand *must* be on the top of the stack.
   moveToTop(Op0, I);
 
   // Change the second operand to the stack register that the operand is in.
   // Change from the pseudo instruction to the concrete instruction.
   MI.RemoveOperand(0);
   MI.RemoveOperand(1);
   MI.getOperand(0).setReg(getSTReg(Op1));
+  MI.getOperand(0).setIsRenamable(false);
   MI.setDesc(TII->get(getConcreteOpcode(MI.getOpcode())));
 
   // If we kill the second operand, make sure to pop it from the stack.
   if (Op0 != Op1 && KillsOp1) {
     // Get this value off of the register stack.
     freeStackSlotAfter(I, Op1);
   }
 }
for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
       unsigned FPReg = getFPReg(Op);
 
       if (FRegIdx.count(i))
         // Operand with constraint "f".
         Op.setReg(getSTReg(FPReg));
       else
         // Operand with a single register class constraint ("t" or "u").
         Op.setReg(X86::ST0 + FPReg);
+      Op.setIsRenamable(false);
     }
 
     // Simulate the inline asm popping its inputs and pushing its outputs.
     StackTop -= NumSTPopped;
 
     for (unsigned i = 0; i < NumSTDefs; ++i)
       pushReg(NumSTDefs - i - 1);
 

test/CodeGen/Mips/sll-micromips-r6-encoding.mir

frameInfo:
   hasMustTailInVarArgFunc: false
   savePoint:       ''
   restorePoint:    ''
 fixedStack:
 stack:
 constants:
 body:             |
   bb.0.entry:
-    renamable %zero = SLL_MMR6 killed renamable %zero, 0
+    %zero = SLL_MMR6 killed %zero, 0
     JRC16_MM undef %ra, implicit %v0
 
 ...