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

[Tri Vo via Phabricator via llvm-commits]

trong marked an inline comment as done.
trong 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);
----------------
nickdesaulniers wrote:
> 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.
Since CSR masks and CSR lists have different format, we have to deal with them differently. 


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


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7779-7780
       Subtarget->getRegisterInfo()->getWindowsStackProbePreservedMask();
+  if (Subtarget->hasCustomCallingConv())
+    UpdateMaskCustomCall(DAG.getMachineFunction(), &Mask);
 
----------------
nickdesaulniers wrote:
> 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)`?
Even though hasCustomCallingConv() checks are repetitive, I'd like to keep the check outside of `UpdateMaskCustomCall` for readability of instruction selection code.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.h:550
 
+  void UpdateMaskCustomCall(MachineFunction &MF, const uint32_t **Mask) const;
+
----------------
nickdesaulniers wrote:
> 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>()`.
`Subtarget` only exposes target information, so I don't think  `Subtarget` is good place to make changes to `MF`. Added helper functions to AArch64RegisterInfo (not ideal either) so that they could be used by both isel implementations.


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:82-83
+      SmallVectorImpl<MCPhysReg> &UpdatedCSRs) const {
+  for (const MCPhysReg *I = CSRs; *I; ++I)
+    UpdatedCSRs.push_back(*I);
+
----------------
nickdesaulniers wrote:
> Can [[ http://www.cplusplus.com/reference/algorithm/copy/ | std::copy ]] be used here?
We rely on last value of the list being 0. We'd have to look for the end of the list first to use `std::copy`, which is not as nice as looping through just once.


================
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));
+    }
+  }
----------------
nickdesaulniers wrote:
> Can [[ http://www.cplusplus.com/reference/algorithm/copy_if/ | std::copy_if ]] be used here?
We don't have direct access to the copied container.


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


================
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),
----------------
nickdesaulniers wrote:
> 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.
E.g. w0 (32-bit) is a subregister of x0 (64-bit)
CSR masks account for all subregisters:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/CodeGen/TargetRegisterInfo.h#L471
So while looping over X registers we also need to set the bit for the corresponding W register.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D52216