[PATCH] D28566: Disable Callee Saved Registers

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:212
+  /// I.e. the register will not appear as part of the CSR register mask.
+  /// The function is lazy allocating a the updated lisy of CSR
+  /// (UpdatedCalleeSavedRegs).
----------------
Typos:

CSR - Callee Saved Registers.
So, do not write CSR register mask, either say "CSR mask", or "callee saved register mask"

"allocating a the updated lisy" -> "allocating the updated list"


================
Comment at: include/llvm/CodeGen/RegisterClassInfo.h:59
+  // Map register alias to the callee saved Register.
+  SmallVector<uint8_t, 4> CalleeSavedAliases;
 
----------------
should be:

```
SmallVector<MCPhysReg, 4> CalleeSavedAliases;
```
In the previous implementation CSRNum[i] was an index into CalleeSaved, and the assumption was that CalleeSaved will not have more than 2^8 = 256 entries.
This is not the case with your new implementation where CalleeSavedAliases[i] is not an index into CalleeSavedRegs, but the actual MCPhysReg.


================
Comment at: lib/CodeGen/LivePhysRegs.cpp:180
   const MachineFunction &MF = *MBB.getParent();
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
   const MachineFrameInfo &MFI = MF.getFrameInfo();
----------------
You may move this down to line 185, just before the line it is used at.


================
Comment at: lib/CodeGen/MachineInstr.cpp:267
+    // Calculate the size of the RegMask
+    const MachineFunction *MF = getParent()->getParent()->getParent();
+    const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
----------------
It is still worth to check if both have the same pointer, in such case we can return true.
Only if we have different pointers we need to check each element.


```
if(getRegMask() == Other.getRegMask())
  return true;
```


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:561
+  if (UpdatedCalleeSavedRegs == nullptr)
+    UpdatedCalleeSavedRegs = MF->allocateRegisterList(TRI->getNumRegs() + 1);
+
----------------
UpdatedCalleeSavedRegs can be of type SmallVector<MCPhysReg, 16>.
And at getUpdatedCalleeSavedRegs just return UpdatedCalleeSavedRegs.data();

In this case you can get rid of all these:

```
allocateRegisterList
CSRVector
CalleeSaveDisableRegs

```

Just make sure that last entry in UpdatedCalleeSavedRegs is a zero.
And you can remove registers from this buffer using 

```
UpdatedCalleeSavedRegs.erase(std::remove(UpdatedCalleeSavedRegs.begin(), UpdatedCalleeSavedRegs.end(), Reg)
```

There will not be many registers to remove, and we only add registers once, at first time we entry this function.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3721
+  // Define a new register mask from the existing mask.
+  uint32_t *RegMask;
+
----------------
Initialize this to nullptr.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3746
+  // Create the RegMask Operand according to our updated mask.
+  Ops.push_back(DAG.getRegisterMask(RegMask));
 
----------------
sink this into the above if-else.
This case, there will be no case where we need to do const_cast, and for sure will not be sending the static "Mask" as "RegMask" to the LowerCallResult function.


Repository:
  rL LLVM

https://reviews.llvm.org/D28566