[PATCH] D76130: [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX in 32-bit mode.

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: clang/lib/Basic/Targets/PPC.h:373
+    // This is the ELF definition, and is overridden by the Darwin and AIX
+    // sub-target
     return TargetInfo::PowerABIBuiltinVaList;
----------------
nit: add '.' to the comment while we are at it.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7067
+
+      const static MCPhysReg GPR_32[] = {PPC::R3, PPC::R4, PPC::R5, PPC::R6,
+                                         PPC::R7, PPC::R8, PPC::R9, PPC::R10};
----------------
The style in this file is "static const" instead of "const static".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7029
       continue;
+    } else if (VA.isMemLoc()) {
+      const unsigned LocSize = LocVT.getStoreSize();
----------------
Would it be better if we remove the "else if" and make it 

if (VA.isMemloc()){
...
continue;
}

Since this matches the last if statement, and also the style in LowerCall_AIX.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7038
+        CurArgOffset += LocSize - ValSize;
+      MachineFrameInfo &MFI = MF.getFrameInfo();
+      // Potential tail calls could cause overwriting of argument stack slots.
----------------
We could remove this definition if we have one defined outside.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7077
+
+      FuncInfo->setVarArgsNumGPR(
+          CCInfo.getFirstUnallocated(IsPPC64 ? GPR_64 : GPR_32));
----------------
This only gets used by 32bit-SVR4 target, I don't think we need to set it. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7082
+      SDValue FIN = DAG.getFrameIndex(FuncInfo->getVarArgsFrameIndex(), PtrVT);
+      // The fixed integer arguments of a variadic function are stored to the
+      // VarArgsFrameIndex on the stack so that they may be loaded by
----------------
start a new line before the comment. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7088
+           GPRIndex < NumGPArgRegs; ++GPRIndex) {
+        unsigned VReg = MF.getRegInfo().getLiveInVirtReg(
+            IsPPC64 ? GPR_64[GPRIndex] : GPR_32[GPRIndex]);
----------------
Just curious, in what scenario would we already have the GPR stored in a virtual register before we store that GPR on that stack?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:9
+; Function Attrs: argmemonly nounwind willreturn
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) #1
+
----------------
I'm not seeing any attribute here, so I'm assuming every "#number" and comments about "Function Attrs" in the test could be removed. 


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

https://reviews.llvm.org/D76130