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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked an inline comment as done.
gberry added a comment.

I made the comment less strong and added a test case.  Let me know what you think about the addOperand issue.



================
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);
----------------
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.


https://reviews.llvm.org/D39400