[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

6034

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

6034

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

3923–3929

Should we check the ELFV2ABI here?

Also type check?

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

3924

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

3923–3929

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.

3924

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

static Align CalculateStackSlotAlignment(EVT ArgVT, EVT OrigVT,
 
   return Alignment;
 }
 
 /// CalculateStackSlotUsed - Return whether this argument will use its
 /// stack slot (instead of being passed in registers).  ArgOffset,
 /// AvailableFPRs, and AvailableVRs must hold the current argument
 /// position, and will be updated to account for this argument.
-static bool CalculateStackSlotUsed(EVT ArgVT, EVT OrigVT, ISD::ArgFlagsTy Flags,
+static bool CalculateStackSlotUsed(const PPCSubtarget &Subtarget, EVT ArgVT,
+                                   EVT OrigVT, ISD::ArgFlagsTy Flags,
                                    unsigned PtrByteSize, unsigned LinkageSize,
                                    unsigned ParamAreaSize, unsigned &ArgOffset,
                                    unsigned &AvailableFPRs,
                                    unsigned &AvailableVRs) {
   bool UseMemory = false;
 
   // Respect alignment of argument on the stack.
   Align Alignment =
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;
       }
-  }
+  } else if (Subtarget.isPPC64() && Subtarget.isELFv2ABI() &&
+             Flags.getByValSize() >= 8)
+    // For 64-bit ELF v2, passing by value object whose size is no less than 8
+    // bytes will be copied to parameter save area. This is for compatibility
+    // for other compiler which requires byval parameters to be stored in
+    // caller's parameter save area.
+    return true;
 
   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) {
SDValue PPCTargetLowering::LowerFormalArguments_64SVR4(
   unsigned ParamAreaSize = Num_GPR_Regs * PtrByteSize;
   unsigned NumBytes = LinkageSize;
   unsigned AvailableFPRs = Num_FPR_Regs;
   unsigned AvailableVRs = Num_VR_Regs;
   for (unsigned i = 0, e = Ins.size(); i != e; ++i) {
     if (Ins[i].Flags.isNest())
       continue;
 
-    if (CalculateStackSlotUsed(Ins[i].VT, Ins[i].ArgVT, Ins[i].Flags,
+    if (CalculateStackSlotUsed(Subtarget, Ins[i].VT, Ins[i].ArgVT, Ins[i].Flags,
                                PtrByteSize, LinkageSize, ParamAreaSize,
                                NumBytes, AvailableFPRs, AvailableVRs))
       HasParameterArea = true;
   }
 
   // Add DAG nodes to load the arguments or copy them out of registers.  On
   // entry to a function on PPC, the arguments start after the linkage area,
   // although the first ones are often in registers.
needStackSlotPassParameters(const PPCSubtarget &Subtarget,
 
   unsigned NumBytes = LinkageSize;
   unsigned AvailableFPRs = NumFPRs;
   unsigned AvailableVRs = NumVRs;
 
   for (const ISD::OutputArg& Param : Outs) {
     if (Param.Flags.isNest()) continue;
 
-    if (CalculateStackSlotUsed(Param.VT, Param.ArgVT, Param.Flags, PtrByteSize,
-                               LinkageSize, ParamAreaSize, NumBytes,
-                               AvailableFPRs, AvailableVRs))
+    if (CalculateStackSlotUsed(Subtarget, Param.VT, Param.ArgVT, Param.Flags,
+                               PtrByteSize, LinkageSize, ParamAreaSize,
+                               NumBytes, AvailableFPRs, AvailableVRs))
       return true;
   }
   return false;
 }
 
 static bool hasSameArgumentList(const Function *CallerFn, const CallBase &CB) {
   if (CB.arg_size() != CallerFn->arg_size())
     return false;
SDValue PPCTargetLowering::LowerCall_64SVR4(
   bool HasParameterArea = !isELFv2ABI || CFlags.IsVarArg || IsFastCall;
   if (!HasParameterArea) {
     unsigned ParamAreaSize = NumGPRs * PtrByteSize;
     unsigned AvailableFPRs = NumFPRs;
     unsigned AvailableVRs = NumVRs;
     unsigned NumBytesTmp = NumBytes;
     for (unsigned i = 0; i != NumOps; ++i) {
       if (Outs[i].Flags.isNest()) continue;
-      if (CalculateStackSlotUsed(Outs[i].VT, Outs[i].ArgVT, Outs[i].Flags,
-                                 PtrByteSize, LinkageSize, ParamAreaSize,
-                                 NumBytesTmp, AvailableFPRs, AvailableVRs))
+      if (CalculateStackSlotUsed(Subtarget, Outs[i].VT, Outs[i].ArgVT,
+                                 Outs[i].Flags, PtrByteSize, LinkageSize,
+                                 ParamAreaSize, NumBytesTmp, AvailableFPRs,
+                                 AvailableVRs))
         HasParameterArea = true;
     }
   }
 
   // When using the fast calling convention, we don't provide backing for
   // arguments that will be in registers.
   unsigned NumGPRsUsed = 0, NumFPRsUsed = 0, NumVRsUsed = 0;
 
if (IsFastCall) {
         case MVT::f64:
           if (++NumFPRsUsed <= NumFPRs)
             continue;
           break;
         }
         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())
       NumBytes = ((NumBytes + PtrByteSize - 1)/PtrByteSize) * PtrByteSize;

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
 }