[PowerPC][ELF]make sure local variable space does not overlap with parameter save area

[shchenz]

Summary

This is a bug fix for byval parameter passing in the caller site.

Currently in the caller site, byval parameter which size is not smaller than 8 will be stored into parameter save area.

See:

LowerCall_64SVR4  -> createMemcpyOutsideCallSeq()

But in the previous HasParameterArea checking, it is not set to true. So this will cause the stack memory for local variable space and parameter save area overlap.

[shchenz]

delete the auto gen comments in testcase.

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

6019

This doesn't look like a correct fix to me.
I think we rely on CalculateStackSlotUsed for non FastCall ELFV2ABI.
Do we know why CalculateStackSlotUsed is returning false for this case?

[shchenz]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

6019

Thanks, I will do more investigation. Actually, I find the stack size is unnecessary bigger than needed after the fix.

[shchenz]

1: the unexpected size is caused by: there is a minimal parameter save area requirement which is 64 bytes. It is bigger than the needed 36 bytes.

2: Because the parameter size is 36 bytes, it can be passed by registers, so current CalculateStackSlotUsed return false. But because the byval parameter may be assumed in caller's parameter save area in the callee in other compiler, so we must also store the byval parameter in parameter save area of the caller function. Maybe we need a revisit for this case. For my understanding, there is no need to store byval parameter in the parameter save area in the caller site if the parameters can be passed in registers. Callee site should copy the byval parameter to its own stack memory, so any operation will not impact the caller site. Not sure which gcc version requires this and from the later comments, seems this is not implemented? https://github.com/llvm/llvm-project/blob/21fd8759529707b0f4430ebe8f27a01edc7f655e/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L6172-L6181 But this should be another improvement issue from this functionality one.

[jsji]

This comment was removed by jsji.

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

3915

Should we check the ELFV2ABI here?

Also type check?

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

3916

comment are referring to the opposite? Should we describe the ABI about size >=8?

[shchenz]

1: add explicitly check for abi

[shchenz]

Thanks for the review @jsji. Updated accordingly.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

3915

I added ABI check, I think we don't need the type check, as here the type is byval and it is always a pointer to a structure or a scalar type, we just need to know its size.

3916

I think saving the byval parameter (size>=8bytes) into the stack is not required by ABI. This is for compatibility with GCC. See https://github.com/llvm/llvm-project/blob/21fd8759529707b0f4430ebe8f27a01edc7f655e/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L6172-L6181.
I think we need to reinvestigate this requirement for byval parameter on Linux. Saving byval parameter in caller's stack frame is not necessary IMO, for example, we don't do this on AIX. And from the latter comments of the above codes, GCC seems also does not require this in its implementation

[shchenz]

gentle ping

[shchenz]

gentle ping

[shchenz]

gentle ping

[jsji]

LGTM. Sorry for missing pings .

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

if (!Flags.isByVal()) {
     if (ArgVT == MVT::v4f32 || ArgVT == MVT::v4i32 ||
         ArgVT == MVT::v8i16 || ArgVT == MVT::v16i8 ||
         ArgVT == MVT::v2f64 || ArgVT == MVT::v2i64 ||
         ArgVT == MVT::v1i128 || ArgVT == MVT::f128)
       if (AvailableVRs > 0) {
         --AvailableVRs;
         return false;
       }
   }
 
   return UseMemory;
 }
 
 /// EnsureStackAlignment - Round stack frame size up from NumBytes to
 /// ensure minimum alignment required for target.
 static unsigned EnsureStackAlignment(const PPCFrameLowering *Lowering,
                                      unsigned NumBytes) {
   return alignTo(NumBytes, Lowering->getStackAlign());
if (IsFastCall) {
         case MVT::f32:
         case MVT::f64:
           if (++NumFPRsUsed <= NumFPRs)
             continue;
           break;
         }
         HasParameterArea = true;
       }
-    }
+    } else if (Flags.isByVal() && Flags.getByValSize() >= 8)
+      HasParameterArea = true;
 
     /* Respect alignment of argument on the stack.  */
     auto Alignement =
         CalculateStackSlotAlignment(ArgVT, OrigVT, Flags, PtrByteSize);
     NumBytes = alignTo(NumBytes, Alignement);
 
     NumBytes += CalculateStackSlotSize(ArgVT, Flags, PtrByteSize);
     if (Flags.isInConsecutiveRegsLast())

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 byval parameter %x, make sure the memory for local variable and
+; for parameter save area are not overlap.
 ; For the below case,
-; the local variable space is r1 + 40 ~ r1 + 76
+; the local variable space is r1 + 104 ~ r1 + 140
 ; the parameter save area is r1 + 32 ~ r1 + 68
 
 define dso_local i32 @bar() {
 ; CHECK-LABEL: bar:
-; CHECK:    addi 30, 1, 40
+; CHECK:    addi 30, 1, 104
 ; CHECK:    li 3, 16
 ; CHECK:    lxvd2x 0, 30, 3
 ; CHECK:    li 3, 48
 ; CHECK:    stxvd2x 0, 1, 3
 ; CHECK:    li 3, 32
 ; CHECK:    lxvd2x 0, 0, 30
 ; CHECK:    stxvd2x 0, 1, 3
-; CHECK:    lwz 3, 72(1)
+; CHECK:    lwz 3, 136(1)
 ; CHECK:    stw 3, 64(1)
 entry:
   %x = alloca %struct, align 4
   call void @foo(%struct* %x)
   %r = call i32 @foo1(%struct* byval(%struct) %x)
   ret i32 %r
 }