[PATCH] D52216: [AArch64] Support adding X[8-15, 18] registers as CSRs.

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: lib/Target/AArch64/AArch64CallLowering.cpp:343
+    const MCPhysReg *CSRs = TRI->getCalleeSavedRegs(&MF);
+    SmallVector<MCPhysReg, 32> UpdatedCSRs;
+    TRI->getCustomCalleeSavedRegs(MF, CSRs, UpdatedCSRs);
----------------
This looks very very similar to `UpdateMaskCustomCall`. Is there a way to convert a `uint32_t[]` to a `SmallVector<MCPhysReg, 32>`? That might help cut down on code duplication further.


================
Comment at: lib/Target/AArch64/AArch64CallLowering.cpp:390-394
+  if (MF.getSubtarget<AArch64Subtarget>().hasCustomCallingConv()) {
+    uint32_t *UpdatedMask = MF.allocateRegMask();
+    TRI->getCustomCallPreservedMask(MF, Mask, UpdatedMask);
+    Mask = UpdatedMask;
+  }
----------------
`UpdateMaskCustomCall`?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3110
+  if (Subtarget->hasCustomCallingConv()) {
+    auto TRI = MF.getSubtarget<AArch64Subtarget>().getRegisterInfo();
+    const MCPhysReg *CSRs = TRI->getCalleeSavedRegs(&MF);
----------------
Don't we already have a reference to the Subtarget on the previous line? `auto TRI = Subtarget.getRegisterInfo();`?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7779-7780
       Subtarget->getRegisterInfo()->getWindowsStackProbePreservedMask();
+  if (Subtarget->hasCustomCallingConv())
+    UpdateMaskCustomCall(DAG.getMachineFunction(), &Mask);
 
----------------
It seems like all invocations of `UpdateMaskCustomCall` have this pattern:

`if (x) UpdateMaskCustomCall(...);`

where x is the same condition.  Can the condition be moved in to `UpdateMaskCustomCall` and then `UpdateMaskCustomCall` called unconditionally, only actually doing anything `if (x)`?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.h:550
 
+  void UpdateMaskCustomCall(MachineFunction &MF, const uint32_t **Mask) const;
+
----------------
Is there somewhere this can go so that it can continue to be used from `lib/Target/AArch64/AArch64ISelLowering.cpp` but additionally `lib/Target/AArch64/AArch64CallLowering.cpp`?  See above comment.

It seems like `Subtarget` is accessible through the provided `MF` via `MF.getSubtarget<AArch64Subtarget>()`.


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:82-83
+      SmallVectorImpl<MCPhysReg> &UpdatedCSRs) const {
+  for (const MCPhysReg *I = CSRs; *I; ++I)
+    UpdatedCSRs.push_back(*I);
+
----------------
Can [[ http://www.cplusplus.com/reference/algorithm/copy/ | std::copy ]] be used here?


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:85-89
+  for (size_t i = 0; i < AArch64::GPR64commonRegClass.getNumRegs(); ++i) {
+    if (MF.getSubtarget<AArch64Subtarget>().isXRegCustomCalleeSaved(i)) {
+      UpdatedCSRs.push_back(AArch64::GPR64commonRegClass.getRegister(i));
+    }
+  }
----------------
Can [[ http://www.cplusplus.com/reference/algorithm/copy_if/ | std::copy_if ]] be used here?


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:92
+  UpdatedCSRs.push_back(0);
+  return;
+}
----------------
no need for empty return.


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:149-151
+  for (size_t i = 0; i < AArch64::GPR64commonRegClass.getNumRegs(); ++i) {
+    if (MF.getSubtarget<AArch64Subtarget>().isXRegCustomCalleeSaved(i)) {
+      for (MCSubRegIterator SubReg(AArch64::GPR64commonRegClass.getRegister(i),
----------------
Just trying to understand what `SubReg` is in this case?  I would think we only need to iterate over the number of registers once, so I don't understand the inner for loop.  I've read the comment above the declaration of `MCSubRegIterator`, but I guess I don't understand what a subreg is in context of aarch64.


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:154
+           SubReg.isValid(); ++SubReg) {
+        // See TargetRegisterInfo::getCallPreserverMask for how to interpret the
+        // register mask.
----------------
grep did not turn up matches for this method.  Typo, or was a function you added then renamed?


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:160
+  }
+  return;
+}
----------------
Not an expert on the LLVM coding style, but no need for empty `return`s in `void` returning functions.


Repository:
  rL LLVM

https://reviews.llvm.org/D52216