[PATCH] D28566: Disable Callee Saved Registers

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I completely agree with Quentin that we have to change the comment (or even better the name) of `TargetRegisterInfo::getUpdatedCalleeSavedRegs()` to discourage users to use the "wrong" function.



================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:220
+  /// vector.
+  void addCalleeSaveDisableRegister(unsigned Reg) {
+    assert(Reg < CalleeSaveDisableRegs.size() &&
----------------
Maybe `disableCalleeSavedRegister()` and improve the comment to explain the effects this has for a user (rather than explaining that a bit is set in a private datastructure).


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:222
+    assert(Reg < CalleeSaveDisableRegs.size() &&
+           "Trying to disable an invalif register");
+    const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
----------------
Typo: invalif


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:229-233
+  // The function returns an updated CSR list (after taking into account
+  // registers that are disabled from the CSR list).
+  // The function is lazy allocating the new list and saves it in
+  // UpdatedCalleeSavedRegs.
+  const MCPhysReg *getUpdatedCalleeSavedRegs();
----------------
Should be a doxygen comment (`///`)

This seems to be THE way to get a list of callee saved registers now. So at least the brief description should reflect that and avoid words like "updated" or explanations on what is happening internally. I'd expect something like this (the lazy allocation bits seem like an implementation detail that can be left out):
```
/// Return list of callee saved registers.
///
/// This returns targets default callee saved register list without the registers explicitely disabled by disableCalleeSavedRegister().
```


================
Comment at: include/llvm/CodeGen/RegisterClassInfo.h:56
+  // runOnFunction() call. Used only to determine if an update was made.
+  SmallVector<MCPhysReg, 16> CalleeSavedRegs;
 
----------------
I didn't follow the code completely, but do we still need an extra SmallVector if there is MachineRegisterInfo::getUpdatedCalleeSavedRegs() that allocates storage itself?


================
Comment at: lib/CodeGen/AggressiveAntiDepBreaker.cpp:161-162
 
-  // Mark live-out callee-saved registers. In a return block this is
+  // Mark live-out callee-saved registers (that are not
+  // passed/returned as arguments). In a return block this is
   // all callee-saved registers. In non-return this is any
----------------
The remark in braces is probably not necessary? My understanding is that registers that pass/return arguments simply are not callee saved registers so there shouldn't be a need to mention this here.


================
Comment at: lib/CodeGen/CriticalAntiDepBreaker.cpp:69-70
 
-  // Mark live-out callee-saved registers. In a return block this is
+  // Mark live-out callee-saved registers (that are not
+  // passed/returned as arguments). In a return block this is
   // all callee-saved registers. In non-return this is any
----------------
dito.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:550-551
+const MCPhysReg *MachineRegisterInfo::getUpdatedCalleeSavedRegs() {
+  if (UpdatedCalleeSavedRegs != nullptr)
+    return UpdatedCalleeSavedRegs;
+
----------------
Could you move the lazy memory allocation to the addDisableCalleeSavedReg() function so this can function can become const?

There are a bunch of not so nice `const_cast`s now because of this.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3198-3200
+  for (MachineRegisterInfo::livein_iterator I = MF.getRegInfo().livein_begin(),
+                                            E = MF.getRegInfo().livein_end();
+       I != E; I++)
----------------
```
const MachineRegisterInfo &MRI = MF.getRegInfo();
for (std::pair<unsigned,unsigned> P : make_range(MRI.livein_begin(), MRI.livein_end()) { ... }
```?


Repository:
  rL LLVM

https://reviews.llvm.org/D28566