[PowerPC] Enable sret arguments.

[sfertile]

Summary

Removes the fatal error for sret arguments and adds lit testing.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

37

What is the cause of this alignment value?

[ZarkoCA]

So it doesn't look like we need to add any special handling for struct returns on AIX? I had a quick look and none of the other PPC targets seem to have anything.

[hubert.reinterpretcast]

In D71504#1785698, @ZarkoCA wrote:

So it doesn't look like we need to add any special handling for struct returns on AIX? I had a quick look and none of the other PPC targets seem to have anything.

By the time we get the IR in this form from the front-end, we shouldn't need special handling here. We know the front-end does not represent complex types in an ideal way for AIX here.

[sfertile]

In D71504#1786135, @hubert.reinterpretcast wrote:

In D71504#1785698, @ZarkoCA wrote:

So it doesn't look like we need to add any special handling for struct returns on AIX? I had a quick look and none of the other PPC targets seem to have anything.

By the time we get the IR in this form from the front-end, we shouldn't need special handling here. We know the front-end does not represent complex types in an ideal way for AIX here.

We added the fatal_error in the previous patch in order to keep the patch as small and focused as possible. For back-end implementation everything should just work: we allocate a frame index for the object on the stack and pass a pointer to the frame-object as the argument. At that point its simply another pointer argument. As Hubert mentions though there is front-end (clang/CodeGen) work that is needed for generating the correct IR for AIX argument passing.

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

37

Frame object alignment smaller then 8 gets promoted ... but I'm not exactly sure why 8 as opposed to 4 or 16. I'll figure out where the 8 comes from.

[ZarkoCA]

We added the fatal_error in the previous patch in order to keep the patch as small and focused as possible. For back-end implementation everything should just work: we allocate a frame index for the object on the stack and pass a pointer to the frame-object as the argument. At that point its simply another pointer argument. As Hubert mentions though there is front-end (clang/CodeGen) work that is needed for generating the correct IR for AIX argument passing.

I don't know what happens in the case where the front-end doesn't handle some cases correctly but the back-end does. Is there a danger in enabling this in the back-end knowing we don't get correct IR for complex types for AIX? Is there an error in the front-end for those before we get to this step?

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

1–3

Do we need '-verify-machineinstrs` in these tests?

[hubert.reinterpretcast]

In D71504#1787758, @ZarkoCA wrote:

I don't know what happens in the case where the front-end doesn't handle some cases correctly but the back-end does. Is there a danger in enabling this in the back-end knowing we don't get correct IR for complex types for AIX? Is there an error in the front-end for those before we get to this step?

There's no error from the front-end. In terms of continuous delivery, this leads to silently bad code that could lead to incorrect output or crashes at runtime.

[sfertile]

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

1–3

Yes, good call.

37

When determining the alignment of the frame object we consider 3 different alignments and take the maximum of those. The three alignments are

1. The preferred aggregate alignment which is 8 by default: here
2. The alignment from the StructLayout object.
3. The alignment on the alloca instruction.

So any alignment less then 8 will get promoted to 8 when creating the frame object.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

37

Got it. Thanks for looking into this.

[sfertile]

Rebased and added '-verify-machineinstrs` to the run commands.

[sfertile]

In D71504#1787912, @hubert.reinterpretcast wrote:

In D71504#1787758, @ZarkoCA wrote:

I don't know what happens in the case where the front-end doesn't handle some cases correctly but the back-end does. Is there a danger in enabling this in the back-end knowing we don't get correct IR for complex types for AIX? Is there an error in the front-end for those before we get to this step?

There's no error from the front-end. In terms of continuous delivery, this leads to silently bad code that could lead to incorrect output or crashes at runtime.

I think if the front-end AIX support was further along then a patch like this should be deferred until we fixed (and verified) the IR the front-end generates for sret arguments. With how clang/CodeGen is going through the SVR4 targets for AIX its safe to consider any Clang AIX support as extremely experimental right now, and the back-end implementation shouldn't be held up based on Clang support for the target.

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

37

np.

[hubert.reinterpretcast]

@sfertile, I'm not seeing the change to the test to add -verify-machineinstrs. Perhaps you have a newer version to upload?

[sfertile]

Uploaded wrong diff last time.

[hubert.reinterpretcast]

In D71504#1793240, @sfertile wrote:

I think if the front-end AIX support was further along then a patch like this should be deferred until we fixed (and verified) the IR the front-end generates for sret arguments. With how clang/CodeGen is going through the SVR4 targets for AIX its safe to consider any Clang AIX support as extremely experimental right now, and the back-end implementation shouldn't be held up based on Clang support for the target.

Sounds reasonable. This patch LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

static bool CC_AIX(unsigned ValNo, MVT ValVT, MVT LocVT,
                    CCState &State) {
 
   if (ValVT == MVT::f128)
     report_fatal_error("f128 is unimplemented on AIX.");
 
   if (ArgFlags.isByVal())
     report_fatal_error("Passing structure by value is unimplemented.");
 
-  if (ArgFlags.isSRet())
-    report_fatal_error("Struct return arguments are unimplemented.");
-
   if (ArgFlags.isNest())
     report_fatal_error("Nest arguments are unimplemented.");
 
   const PPCSubtarget &Subtarget = static_cast<const PPCSubtarget &>(
       State.getMachineFunction().getSubtarget());
   const bool IsPPC64 = Subtarget.isPPC64();
   const unsigned PtrByteSize = IsPPC64 ? 8 : 4;
 

llvm/test/CodeGen/PowerPC/aix-sret-param.ll

-; RUN: not llc -mtriple powerpc-ibm-aix-xcoff < %s 2>&1 | FileCheck %s
-; RUN: not llc -mtriple powerpc64-ibm-aix-xcoff < %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec \
+; RUN: -stop-after=machine-cp --verify-machineinstrs < %s | FileCheck \
+; RUN: --check-prefixes=MIR,MIR32 %s
 
-%struct.S = type { i32 }
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec \
+; RUN: -stop-after=machine-cp --verify-machineinstrs < %s | FileCheck \
+; RUN: --check-prefixes=MIR,MIR64 %s
 
-define void @barv() {
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec \
+; RUN: --verify-machineinstrs < %s | FileCheck --check-prefixes=ASM,ASM32 %s
+
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec \
+; RUN: --verify-machineinstrs < %s | FileCheck --check-prefixes=ASM,ASM64 %s
+
+%struct.S = type { i8 }
+%struct.T = type { double, i32, i32, i32, float }
+
+define void @test1() {
+entry:
+  %s = alloca %struct.S, align 4
+  call void @foo(%struct.S* sret %s)
+  ret void
+}
+
+define void @test2() {
 entry:
-  %tmp = alloca %struct.S, align 4
-  call void @foo(%struct.S* sret %tmp)
+  %t = alloca %struct.T, align 8
+  call void @bar(%struct.T* sret %t)
   ret void
 }
 
 declare void @foo(%struct.S* sret)
+declare void @bar(%struct.T* sret)
+
+; MIR:      name:            test1
+; MIR:      stack:
+; MIR-NEXT:  - { id: 0, name: s, type: default, offset: 0, size: 1, alignment: 8,
+
+; MIR32:      bb.0.entry:
+; MIR32-NEXT:   ADJCALLSTACKDOWN 56, 0, implicit-def dead $r1, implicit $r1
+; MIR32-NEXT:   renamable $r3 = ADDI %stack.0.s, 0
+; MIR32-NEXT:   BL_NOP <mcsymbol .foo>, csr_aix32, implicit-def dead $lr, implicit $rm, implicit $r3, implicit $r2, implicit-def $r1
+; MIR32-NEXT:   ADJCALLSTACKUP 56, 0, implicit-def dead $r1, implicit $r1
+
+; MIR64:      bb.0.entry:
+; MIR64-NEXT:   ADJCALLSTACKDOWN 112, 0, implicit-def dead $r1, implicit $r1
+; MIR64-NEXT:   renamable $x3 = ADDI8 %stack.0.s, 0
+; MIR64-NEXT:   BL8_NOP <mcsymbol .foo>, csr_aix64, implicit-def dead $lr8, implicit $rm, implicit $x3, implicit $x2, implicit-def $r1
+; MIR64-NEXT:   ADJCALLSTACKUP 112, 0, implicit-def dead $r1, implicit $r1
+
+
+; ASM-LABEL: .test1:
+
+; ASM32:       stwu 1, -64(1)
+; ASM32-NEXT:  addi 3, 1, 56
+; ASM32-NEXT:  bl .foo
+; ASM32-NEXT:  nop
+; ASM32-NEXT:  addi 1, 1, 64
+
+; ASM64:       stdu 1, -128(1)
+; ASM64-NEXT:  addi 3, 1, 120
+; ASM64-NEXT:  bl .foo
+; ASM64-NEXT:  nop
+; ASM64-NEXT:  addi 1, 1, 128
+
+
+
+; MIR:      name:            test2
+; MIR:      stack:
+; MIR-NEXT:   - { id: 0, name: t, type: default, offset: 0, size: 24, alignment: 8,
+
+; MIR32:       bb.0.entry:
+; MIR32-NEXT:    ADJCALLSTACKDOWN 56, 0, implicit-def dead $r1, implicit $r1
+; MIR32-NEXT:    renamable $r3 = ADDI %stack.0.t, 0
+; MIR32-NEXT:    BL_NOP <mcsymbol .bar>, csr_aix32, implicit-def dead $lr, implicit $rm, implicit $r3, implicit $r2, implicit-def $r1
+; MIR32-NEXT:    ADJCALLSTACKUP 56, 0, implicit-def dead $r1, implicit $r1
+
+; MIR64:      bb.0.entry:
+; MIR64-NEXT:   ADJCALLSTACKDOWN 112, 0, implicit-def dead $r1, implicit $r1
+; MIR64-NEXT:   renamable $x3 = ADDI8 %stack.0.t, 0
+; MIR64-NEXT:   BL8_NOP <mcsymbol .bar>, csr_aix64, implicit-def dead $lr8, implicit $rm, implicit $x3, implicit $x2, implicit-def $r1
+; MIR64-NEXT:   ADJCALLSTACKUP 112, 0, implicit-def dead $r1, implicit $r1
+
+
+; ASM-LABEL: .test2:
+
+; ASM32:        stwu 1, -80(1)
+; ASM32-NEXT:   addi 3, 1, 56
+; ASM32-NEXT:   bl .bar
+; ASM32-NEXT:   nop
+; ASM32-NEXT:   addi 1, 1, 80
+
 
-; CHECK: LLVM ERROR: Struct return arguments are unimplemented.
+; ASM64:        stdu 1, -144(1)
+; ASM64-NEXT:   addi 3, 1, 120
+; ASM64-NEXT:   bl .bar
+; ASM64-NEXT:   nop
+; ASM64-NEXT:   addi 1, 1, 144