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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/MachineVerifier.cpp:1111
+        if (MRI->isReserved(Reg))
+          report("isRenamable set on reserved register", MO, MONum);
         return;
----------------
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.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:182
+                             ? MI.getOperand(Idx2).isRenamable()
+                             : false;
   // If destination is tied to either of the commuted source register, then
----------------
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.)


Repository:
  rL LLVM

https://reviews.llvm.org/D42449