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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: include/llvm/CodeGen/MachineOperand.h:88-89
 
-  /// IsDef/IsImp/IsKill/IsDead flags - These are only valid for MO_Register
-  /// operands.
+  /// IsDef/IsImp/IsDeadOrKill/IsRenamable flags - These are
+  /// only valid for MO_Register opderands.
 
----------------
Written like this it'll probably be part of the `IsDef` comment in doxygen. Maybe just append a "Only valid for MO_Register" to each of the four fields instead?


================
Comment at: include/llvm/CodeGen/MachineOperand.h:103
+  /// For defs: IsDead - True if this register is never used by a subsequent
+  /// instruction.  This is only valid on definitions of registers.
+  bool IsDeadOrKill : 1;
----------------
The last sentence is not true anymore.


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


================
Comment at: include/llvm/CodeGen/MachineOperand.h:107-109
+  /// i.e. that it does not generate a value that is somehow read in a way that
+  /// is not represented by the Machine IR (e.g. to meet an ABI or ISA
+  /// requirement).
----------------
Should we state that this bit only affects physical registers? That way we don't need to set this bit everywhere a vreg is used. And I can't imagine any case where we have a non-renamable vreg or put another way: I do not want to add code dealing with non-renamable vregs.


================
Comment at: include/llvm/CodeGen/MachineOperand.h:310-311
   bool isDead() const {
     assert(isReg() && "Wrong MachineOperand accessor");
-    return IsDead;
+    return IsDeadOrKill & IsDef;
   }
----------------
I assume changing this to assert(isReg() && IsDef()) breaks too much code?


================
Comment at: include/llvm/CodeGen/MachineOperand.h:368-372
+  /// Same as setReg() above, but preserves the current value of isRenamable().
+  /// This allows e.g. the register allocators to create physical register
+  /// operands that are marked as being renamable if they were virtual registers
+  /// before allocation.
+  void setRegKeepRenamable(unsigned Reg);
----------------
See my comment in setReg(), I think we shouldn't have this method.


================
Comment at: include/llvm/CodeGen/MachineOperand.h:419
+  void setIsRenamable(bool Val = true) {
+    assert(isReg() && "Wrong MachineOperand mutator");
+    IsRenamable = Val;
----------------
Maybe add `assert(TargetRegisterInfo::isPhysicalRegister(getReg() && "Renamable flag may only be set for physregs");`


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1221
+
+  // Mark MIR regs as renamable unless they are explicitly marked norename.  We
+  // will later go back through and fix up all the physical regs to be marked as
----------------
- "operands" instead of "MIR regs"?


================
Comment at: lib/CodeGen/MIRParser/MIRParser.cpp:420-428
+  // Fix up physical registers to be marked as norename if we are reading a
+  // MIR file with NoVRegs not set (i.e. from before register allocation).
+  if (!MF.getProperties().hasProperty(
+          MachineFunctionProperties::Property::NoVRegs))
+    for (auto &MBB : MF)
+      for (auto &MI : MBB.instrs())
+        for (auto &MO : MI.operands())
----------------
I'm not a fan of the parsing changing depending on a function property. I think the right thing to do is to encode "renamable" and not "norename" after all.

If you are worried about mir output becoming noisy/hard to read after regalloc with "renamable" flags everywhere, maybe we can shorten "renamable" to some character like `~`?


================
Comment at: lib/CodeGen/MachineInstr.cpp:90-91
 
+  IsRenamable = !TargetRegisterInfo::isPhysicalRegister(Reg);
+
   // Otherwise, we have to change the register.  If this operand is embedded
----------------
I find it surprising that `setReg()` affects a flag as well. I'd rather just modify the regallocators to set the flag explicitely where possible.


================
Comment at: lib/CodeGen/MachineVerifier.cpp:365-366
       MachineFunctionProperties::Property::Selected);
+  areRegsAllocated = MF.getProperties().hasProperty(
+      MachineFunctionProperties::Property::NoVRegs);
 
----------------
NoVRegs should mean exactly that: We are guaranteed that no vregs are used. It does not mean: We are in a pass after register allocation.


================
Comment at: lib/CodeGen/MachineVerifier.cpp:1209-1214
+    // Check isRenamable is correct if we're before RA.
+    if (!areRegsAllocated)
+      if (MO->isRenamable() != TargetRegisterInfo::isVirtualRegister(Reg))
+        report("Virtual registers should be marked isRenamable and physical "
+               "registers should not be.",
+               MO, MONum);
----------------
As stated above I would go for Renamable only ever being set on physregs. That way the rules don't need to be conditional based on "regsAllocated".


https://reviews.llvm.org/D39400