ZarkoCA added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7076
+ static const MCPhysReg GPR_64[] = {PPC::X3, PPC::X4, PPC::X5, PPC::X6,
+ PPC::X7, PPC::X8, PPC::X9, PPC::X10};
+ unsigned const NumGPArgRegs = array_lengthof(GPR_32);
----------------
jasonliu wrote:
> This two array gets used to in CC_AIX and here. It would be great if we only
> have one set of copy.
I agree, but the array is local to CC_AIX as far as I understand and there is
not a good way to access it from here without having to modify CC_AIX. I think
it would be a good to do it at a later point when it's not undergoing so much
change already.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
if (VA.isRegLoc()) {
MVT::SimpleValueType SVT = ValVT.getSimpleVT().SimpleTy;
----------------
jasonliu wrote:
> Do we need `&& !VA.needsCustom() ` here?
Yes, it's better if I add it, thanks.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:1
+; RUN: llc -O2 -mtriple powerpc-ibm-aix-xcoff -stop-after=machine-cp
-mattr=-altivec -verify-machineinstrs < %s | \
+; RUN: FileCheck --check-prefixes=CHECK,32BIT %s
----------------
jasonliu wrote:
> Do we need '-O2' specified here and below?
It makes the IR and assembly much cleaner and easier to read, imo.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:7
+; RUN: FileCheck --check-prefixes=CHECKASM,ASM32PWR4 %s
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
----------------
jasonliu wrote:
> A general comment about how to test va_arg.
> I think the two test cases we have here are not enough.
> On top of my head I would probably add in getting double as va_arg argument,
> uses of va_copy and so on.
> It's probably worth checking how other targets test va_arg. Grep "va_start"
> in llvm/test/CodeGen would give you a lot of hit.
Yes, I can clean up the tests and add more. Agreed we need more and I can
simplify them so it's clear what's being tested.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:15
+
+@a = local_unnamed_addr global i32 1, align 4
+@b = local_unnamed_addr global i32 2, align 4
----------------
jasonliu wrote:
> These variables didn't get used anywhere.
Removed, but as I said earlier, I'll also do a rework of the tests.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:27
+; Function Attrs: nounwind
+define i32 @va_arg1(i32 %a, ...) local_unnamed_addr {
+entry:
----------------
jasonliu wrote:
> These tests used for loops to call va_arg mutliple times.
> Maybe it's just me, but I found the test cases are much harder to read.
> I would suggest maybe just unroll the for loop and generate as much as the
> va_arg call that you need?
>
> For the cases below, I suppose you would want to call va_arg 9 times, and 2
> times respectfully. Maybe it's not that bad. If the tests gets too long, you
> could consider split the tests into separate files.
Yes, see reply above, I will rework the tests.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:33
+ call void @llvm.va_start(i8* nonnull %0)
+ %cmp7 = icmp sgt i32 %a, 0
+ br i1 %cmp7, label %for.body.preheader, label %for.end
----------------
jasonliu wrote:
> a is 1, so we would only call va_arg once, despite we have 9 vaarg argument.
> Is this the intention of the test?
a = 1 doesn't mean we call va_arg once. This is the IR for the function
definition that I got using this c code (modified for this example):
```
a = 1;
int va_arg1(int a,...) {
...
}
```
The callee doesn't know how many vararg arguments will be called, just does the
setup for the variadic parameter (...) in the IR.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:162
+ %add7 = add nsw i32 %add6, %eight
+ %cmp15 = icmp sgt i32 %eight, 0
+ br i1 %cmp15, label %for.body.preheader, label %for.end
----------------
jasonliu wrote:
> We only have 2 va_arg arguments here, but we called va_arg 8 times? I think
> that's undefined behavior.
Seem my reply above. That said, there is some confusion with respect to tests
and they'll be reworked.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76130/new/
https://reviews.llvm.org/D76130
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
