[PATCH] D39400: WIP: [MachineOperand][MIR] Add isRenamable to MachineOperand.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/CodeGen/MachineInstr.cpp:263-268
+    // Set isRenamable to false if the opcode has extra register allocation
+    // constraints.
+    if (TargetRegisterInfo::isPhysicalRegister(NewMO->getReg()) &&
+        ((NewMO->isDef() && hasExtraDefRegAllocReq()) ||
+         (NewMO->isUse() && hasExtraSrcRegAllocReq())))
+      NewMO->setIsRenamable(false);
----------------
gberry wrote:
> MatzeB wrote:
> > Is this a good idea? Intuitively I would not expect an `addOperand()` function to change the operand. Admittedly someone already broke that rule with the setIsEarlyClobber() above but unless all other solutions are really bad, I'd still prefer not to have this here.
> Without this every bit of code that is builds an instruction whose opcode hasExtraRegAllocReq would need to remember to clear the appropriate renamable bit.  It seems to me that the hasExtraRegAllocReq property is less useful that way.  I suppose we could untie these two properties and ask that users check both the renamable bit and the hasExtraRegAllocReq (with a helper function), in which case the verification that the two properties are consistent goes away.
But unless I miss something, the default is not-renamable anyway so all the random code out there doesn't need to be changed. The only places that set the renamable bit are the regallocators and they already use `MO.setIsRenamableIfNoExtraRegAllocReq()` in your commit. So what other code is there that would set the renamable bit even though it shouldn't?


https://reviews.llvm.org/D39400