[PATCH] D130903: [AArch64][GlobalISel] Lower formal arguments of AAPCS & ms_abi variadic functions.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:553
+
+  static const MCPhysReg GPRArgRegs[] = {AArch64::X0, AArch64::X1, AArch64::X2,
+                                         AArch64::X3, AArch64::X4, AArch64::X5,
----------------
It looks like these arrays already exist in AArch64CallingConvention.cpp; is it possible to use them here? (Or should we?)


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:564
+    if (IsWin64CC) {
+      GPRIdx = MFI.CreateFixedObject(GPRSaveSize, -(int)GPRSaveSize, false);
+      if (GPRSaveSize & 15)
----------------
nit: can you use `static_cast` for greppability?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:565
+      GPRIdx = MFI.CreateFixedObject(GPRSaveSize, -(int)GPRSaveSize, false);
+      if (GPRSaveSize & 15)
+        // The extra size here, if triggered, will always be 8.
----------------
Can you explain why this is the case in a comment? Even referencing some win64 doc would be useful?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:568
+        MFI.CreateFixedObject(16 - (GPRSaveSize & 15),
+                              -(int)alignTo(GPRSaveSize, 16), false);
+    } else
----------------
nit: `static_cast`?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:572
+
+    auto FIN = MIRBuilder.buildFrameIndex(LLT::pointer(0, 64), GPRIdx);
+    auto Offset = MIRBuilder.buildConstant(
----------------
can you pull `LLT::pointer(0, 64)` out into like

```
const LLT p0 = LLT::pointer(0, 64);
```

and use that everywhere?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:574
+    auto Offset = MIRBuilder.buildConstant(
+        MRI.createGenericVirtualRegister(LLT::scalar(64)), 8);
+
----------------
similar to above, can you create a

```
 LLT s64 = LLT::scalar(64);
```

and use that everywhere?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130903/new/

https://reviews.llvm.org/D130903