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

[Geoff Berry via Phabricator via llvm-commits]

gberry 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);
----------------
MatzeB wrote:
> 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?
One example that occurs in the lit tests is code that is constructing a hasExtraReqAllocReq machine instruction after register allocation and copying over operands from an existing instruction.  These particular operands are renamable since they were originally virtual regs.  Once they are added to the new instruction they remain renamable, but are operands of a hasExtraRegAllocReq opcode so they fail verification. 


https://reviews.llvm.org/D39400