[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 ↗

(On Diff #131158)

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 ↗

(On Diff #131158)

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 ↗

(On Diff #131158)

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.

llvm/trunk/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.