[Powerpc] store byval parameter to parameter save area when needed

[shchenz]

Summary

For now, for 64 bit SVR4 ABI, when lowering a CALL, a byval parameter bigger than 8 bytes will be copied to the caller's stack frame. This will cause some issues because when the byval parameter is smaller than 64 bytes (assume there is only one parameter). HasParameterArea is not set to true, so on the caller side, the stack local area overlaps with the parameter save area.

In D105271, I tried to set the HasParameterArea to true even the byval parameter can be entirely passed in registers. That patch can fix the above overlap issue.

But with the fix D105271, we met a new issue when mix-compiling with other compilers like GCC:
On the callee side,
If the callee is compiled with clang and the parameter is set to be with byval parameter in some IR generator, (assume there is only one parameter for the callee and the parameter size is bigger than 8 bytes but smaller than 64 bytes), then the callee assumes the caller will allocate parameter save area, so the callee will store the parameter passed by register to caller's parameter save area.

On the caller side,
If the caller is also compiled with clang, then we are OK as it will use the new logic I added in D105271. Caller and callee have a consistent parameter save area allocation policy.
But if the caller is compiled with other compilers, like GCC, then we will meet some issues. Because GCC will not allocate parameter save area for structures smaller than 64 bytes (assume there is only one parameter).

So I reverted D105271 and propose this new fix.

Now we won't copy the byval parameter (bigger than 8 bytes) to caller's parameter save area. Instead, we will only copy the byval parameter when it can not be passed entirely in registers which means we have to use parameter save area according to the 64 bit SVR4 ABI.

[shchenz]

rebase

[shchenz]

gentle ping

[shchenz]

gentle ping

[shchenz]

ping...
Could someone help to review this one-line change? Or Can I check in this patch first and see if there are any ABI issues exposed?

[shchenz]

gentle ping

[shchenz]

gentle ping

[shchenz]

I asked for some help from a GCC developer. He told me GCC only copies byval structure bigger than 64 bytes to the parameter save area instead of 8 bytes for ELF64. So now after the change, clang should have the same logic as GCC.

[jsji]

LGTM since you have already double checked the compatiblity.

[shchenz]

In D111485#3175267, @jsji wrote:

LGTM since you have already double checked the compatiblity.

Thanks very much. @jsji

The patch passed all our internal tests I can find and I also verified the GCC and commercial XLC's behavior and also confirmed the behavior with the GCC developer.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

if (Flags.isByVal()) {
           AddPtr = DAG.getNode(ISD::ADD, dl, PtrVT, PtrOff, Const);
         }
         Chain = CallSeqStart = createMemcpyOutsideCallSeq(Arg, AddPtr,
                                                           CallSeqStart,
                                                           Flags, DAG, dl);
         ArgOffset += PtrByteSize;
         continue;
       }
-      // Copy entire object into memory.  There are cases where gcc-generated
-      // code assumes it is there, even if it could be put entirely into
-      // registers.  (This is not what the doc says.)
-
-      // FIXME: The above statement is likely due to a misunderstanding of the
-      // documents.  All arguments must be copied into the parameter area BY
-      // THE CALLEE in the event that the callee takes the address of any
-      // formal argument.  That has not yet been implemented.  However, it is
-      // reasonable to use the stack area as a staging area for the register
-      // load.
-
-      // Skip this for small aggregates, as we will use the same slot for a
-      // right-justified copy, below.
-      if (Size >= 8)
+      // Copy the object to parameter save area if it can not be entirely passed 
+      // by registers.
+      // FIXME: we only need to copy the parts which need to be passed in
+      // parameter save area. For the parts passed by registers, we don't need
+      // to copy them to the stack although we need to allocate space for them
+      // in parameter save area.
+      if ((NumGPRs - GPR_idx) * PtrByteSize < Size)
         Chain = CallSeqStart = createMemcpyOutsideCallSeq(Arg, PtrOff,
                                                           CallSeqStart,
                                                           Flags, DAG, dl);
 
       // When a register is available, pass a small aggregate right-justified.
       if (Size < 8 && GPR_idx != NumGPRs) {
         // The easiest way to get this right-justified in a register
         // is to copy the structure into the rightmost portion of a

llvm/test/CodeGen/PowerPC/byval.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512"
 target triple = "powerpc64le-unknown-linux-gnu"
 
 %struct = type { [4 x i32], [20 x i8] }
 
 declare dso_local i32 @foo1(%struct* byval(%struct) %var)
 declare dso_local void @foo(%struct* %var)
 
-; FIXME: for the byval parameter %x, now the memory for local variable and
-; for parameter save area are overlap.
-; For the below case,
-; the local variable space is r1 + 40 ~ r1 + 76
-; the parameter save area is r1 + 32 ~ r1 + 68
+; check that 36bytes byval parameter is passed all in registers.
 
 define dso_local i32 @bar() {
 ; CHECK-LABEL: bar:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    mflr 0
-; CHECK-NEXT:    .cfi_def_cfa_offset 96
-; CHECK-NEXT:    .cfi_offset lr, 16
-; CHECK-NEXT:    .cfi_offset r30, -16
-; CHECK-NEXT:    std 30, -16(1) # 8-byte Folded Spill
 ; CHECK-NEXT:    std 0, 16(1)
-; CHECK-NEXT:    stdu 1, -96(1)
-; CHECK-NEXT:    addi 30, 1, 40
-; CHECK-NEXT:    mr 3, 30
+; CHECK-NEXT:    stdu 1, -80(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 80
+; CHECK-NEXT:    .cfi_offset lr, 16
+; CHECK-NEXT:    addi 3, 1, 40
 ; CHECK-NEXT:    bl foo
 ; CHECK-NEXT:    nop
-; CHECK-NEXT:    li 3, 16
-; CHECK-NEXT:    lxvd2x 0, 30, 3
-; CHECK-NEXT:    li 3, 48
-; CHECK-NEXT:    stxvd2x 0, 1, 3
-; CHECK-NEXT:    li 3, 32
-; CHECK-NEXT:    lxvd2x 0, 0, 30
-; CHECK-NEXT:    stxvd2x 0, 1, 3
-; CHECK-NEXT:    lwz 3, 72(1)
 ; CHECK-NEXT:    ld 7, 72(1)
+; CHECK-NEXT:    ld 6, 64(1)
 ; CHECK-NEXT:    ld 5, 56(1)
 ; CHECK-NEXT:    ld 4, 48(1)
-; CHECK-NEXT:    stw 3, 64(1)
 ; CHECK-NEXT:    ld 3, 40(1)
-; CHECK-NEXT:    ld 6, 64(1)
 ; CHECK-NEXT:    bl foo1
 ; CHECK-NEXT:    nop
-; CHECK-NEXT:    addi 1, 1, 96
+; CHECK-NEXT:    addi 1, 1, 80
 ; CHECK-NEXT:    ld 0, 16(1)
-; CHECK-NEXT:    ld 30, -16(1) # 8-byte Folded Reload
 ; CHECK-NEXT:    mtlr 0
 ; CHECK-NEXT:    blr
 entry:
   %x = alloca %struct, align 4
   call void @foo(%struct* %x)
   %r = call i32 @foo1(%struct* byval(%struct) %x)
   ret i32 %r
 }

llvm/test/CodeGen/PowerPC/elf64-byval-cc.ll

 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    mflr 0
 ; CHECK-NEXT:    std 0, 16(1)
 ; CHECK-NEXT:    stdu 1, -32(1)
 ; CHECK-NEXT:    .cfi_def_cfa_offset 32
 ; CHECK-NEXT:    .cfi_offset lr, 16
 ; CHECK-NEXT:    addis 3, 2, .LC4@toc@ha
 ; CHECK-NEXT:    ld 3, .LC4@toc@l(3)
-; CHECK-NEXT:    ld 4, 0(3)
-; CHECK-NEXT:    std 4, 32(1)
 ; CHECK-NEXT:    ld 3, 0(3)
 ; CHECK-NEXT:    bl test_byval_mem8
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    addi 1, 1, 32
 ; CHECK-NEXT:    ld 0, 16(1)
 ; CHECK-NEXT:    mtlr 0
 ; CHECK-NEXT:    blr
 entry:

 ; CHECK-LABEL: call_test_byval_mem32:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    mflr 0
 ; CHECK-NEXT:    std 0, 16(1)
 ; CHECK-NEXT:    stdu 1, -32(1)
 ; CHECK-NEXT:    .cfi_def_cfa_offset 32
 ; CHECK-NEXT:    .cfi_offset lr, 16
 ; CHECK-NEXT:    addis 3, 2, .LC5@toc@ha
-; CHECK-NEXT:    li 4, 16
 ; CHECK-NEXT:    ld 3, .LC5@toc@l(3)
-; CHECK-NEXT:    lxvd2x 0, 3, 4
-; CHECK-NEXT:    li 4, 48
-; CHECK-NEXT:    stxvd2x 0, 1, 4
-; CHECK-NEXT:    li 4, 32
-; CHECK-NEXT:    lxvd2x 0, 0, 3
-; CHECK-NEXT:    stxvd2x 0, 1, 4
 ; CHECK-NEXT:    ld 6, 24(3)
 ; CHECK-NEXT:    ld 5, 16(3)
 ; CHECK-NEXT:    ld 4, 8(3)
 ; CHECK-NEXT:    ld 3, 0(3)
 ; CHECK-NEXT:    bl test_byval_mem32
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    addi 1, 1, 32
 ; CHECK-NEXT:    ld 0, 16(1)

 ; CHECK-LABEL: call_test_byval_mem32_2:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    mflr 0
 ; CHECK-NEXT:    std 0, 16(1)
 ; CHECK-NEXT:    stdu 1, -32(1)
 ; CHECK-NEXT:    .cfi_def_cfa_offset 32
 ; CHECK-NEXT:    .cfi_offset lr, 16
 ; CHECK-NEXT:    addis 3, 2, .LC5@toc@ha
-; CHECK-NEXT:    li 4, 16
 ; CHECK-NEXT:    addis 8, 2, .LCPI20_0@toc@ha
 ; CHECK-NEXT:    ld 3, .LC5@toc@l(3)
 ; CHECK-NEXT:    lfs 1, .LCPI20_0@toc@l(8)
-; CHECK-NEXT:    lxvd2x 0, 3, 4
-; CHECK-NEXT:    li 4, 56
-; CHECK-NEXT:    stxvd2x 0, 1, 4
-; CHECK-NEXT:    li 4, 40
-; CHECK-NEXT:    lxvd2x 0, 0, 3
-; CHECK-NEXT:    stxvd2x 0, 1, 4
 ; CHECK-NEXT:    ld 7, 24(3)
 ; CHECK-NEXT:    ld 6, 16(3)
 ; CHECK-NEXT:    ld 5, 8(3)
 ; CHECK-NEXT:    ld 4, 0(3)
 ; CHECK-NEXT:    bl test_byval_mem32_2
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    addi 1, 1, 32
 ; CHECK-NEXT:    ld 0, 16(1)

 ; CHECK-LABEL: call_test_byval_mem64:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    mflr 0
 ; CHECK-NEXT:    std 0, 16(1)
 ; CHECK-NEXT:    stdu 1, -32(1)
 ; CHECK-NEXT:    .cfi_def_cfa_offset 32
 ; CHECK-NEXT:    .cfi_offset lr, 16
 ; CHECK-NEXT:    addis 3, 2, .LC6@toc@ha
-; CHECK-NEXT:    li 4, 48
-; CHECK-NEXT:    li 5, 80
-; CHECK-NEXT:    li 6, 64
 ; CHECK-NEXT:    ld 3, .LC6@toc@l(3)
-; CHECK-NEXT:    lxvd2x 0, 3, 4
-; CHECK-NEXT:    stxvd2x 0, 1, 5
-; CHECK-NEXT:    li 5, 32
-; CHECK-NEXT:    lxvd2x 0, 3, 5
-; CHECK-NEXT:    stxvd2x 0, 1, 6
-; CHECK-NEXT:    li 6, 16
-; CHECK-NEXT:    lxvd2x 0, 3, 6
-; CHECK-NEXT:    stxvd2x 0, 1, 4
-; CHECK-NEXT:    lxvd2x 0, 0, 3
-; CHECK-NEXT:    stxvd2x 0, 1, 5
 ; CHECK-NEXT:    ld 10, 56(3)
 ; CHECK-NEXT:    ld 9, 48(3)
 ; CHECK-NEXT:    ld 8, 40(3)
 ; CHECK-NEXT:    ld 7, 32(3)
 ; CHECK-NEXT:    ld 6, 24(3)
 ; CHECK-NEXT:    ld 5, 16(3)
 ; CHECK-NEXT:    ld 4, 8(3)
 ; CHECK-NEXT:    ld 3, 0(3)

llvm/test/CodeGen/PowerPC/ppc64-byval-align.ll

 entry:
   %tmp = alloca %struct.test, align 16
   %.compoundliteral.sroa.0.0..sroa_idx = getelementptr inbounds %struct.test, %struct.test* %tmp, i64 0, i32 0
   store i64 %z, i64* %.compoundliteral.sroa.0.0..sroa_idx, align 16
   %call = call i64 @test2(%struct.pad* byval(%struct.pad) @gp, i32 signext 0, %struct.test* byval(%struct.test) align 16 %tmp)
   ret void
 }
 ; CHECK-LABEL: @caller2
-; CHECK: std 3, [[OFF:[0-9]+]](1)
-; CHECK: addi [[REG1:[0-9]+]], 1, [[OFF]]
-; CHECK: lxvw4x [[REG2:[0-9]+]], 0, [[REG1]]
-; CHECK: li [[REG3:[0-9]+]], 128
-; CHECK: stxvw4x 0, 1, [[REG3]]
-; CHECK: bl test2
+; CHECK-DAG: std 3, [[OFF:[0-9]+]](1)
+; CHECK-DAG: addi [[REG1:[0-9]+]], 1, [[OFF]]
+;
+; CHECK-DAG: lxvw4x [[REG2:[0-9]+]], 0, [[REG1]]
+; CHECK-DAG: li [[REG3:[0-9]+]], 128
+; CHECK:     stxvw4x 0, 1, [[REG3]]
+; CHECK:     bl test2