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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

Thanks, conditional LGTM.

My only remaining issue here is `addOperand()` changing operands. If you can easily fix this just commit without further review, otherwise at least write a comment on why you need it.



================
Comment at: include/llvm/CodeGen/MachineOperand.h:106
 
-  /// IsDead - True if this register is never used by a subsequent instruction.
-  /// This is only valid on definitions of registers.
-  bool IsDead : 1;
+  /// IsRenamable - True if this register may be safely renamed,
+  /// i.e. that it does not generate a value that is somehow read in a way that
----------------
MatzeB wrote:
> One interesting discussion here is how "safe" things really are when this bit is set :)
> I think AMDGPU for example has some extra constraints on physregs that the register allocator isn't aware of, they fix things in a separate pass after allocation AFAIK and after that you better not rename anymore. I think we currently set the `ExtraSrcRegAllocReq` and `ExtraDefRegAllocReq` on those machine instructions to indicate that. Then again we the infrastructure of this of this patch in place we could require them to clear the renamable flag on all affected operands.
I would still vote for a weaker language here by removing the word "safely".


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


https://reviews.llvm.org/D39400