[X86] Fix a bug that when lowering byval argument

[LiuChen3]

Summary

When an argument has 'byval' attribute and should be passed on the stack according calling convention,
a stack copy would be emitted twice. This will cause the real value will be put into stack where the pointer should be passed.

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

4044

Why do we need a new variable? Can we just pass isByVal to the function?

[LiuChen3]

llvm/lib/Target/X86/X86ISelLowering.cpp

4044

Yes, we can. I thought it will reduce readability, so I add a new variable.

[LiuChen3]

Remove redundant variables

[LuoYuanke]

llvm/lib/Target/X86/X86ISelLowering.cpp

3774

Let me check below 2 rule is right or not.

1. On linux when the byval attribute is set, it indicate copy the value that point by the pointer to the parameter stack.
2. On window when the byval attribute is set, it indicate that allocate temporary object in caller, copy the value to the temporary, and store the temporary pointer (which point to the temporary object) to the parameter stack.

On linux, the VA.getLocInfo() is CCValAssign::Full, and on windows is the VA.getLocInfo() is CCValAssign::Indirect.

So I think we can just check the VA.getLocInfo(). If VA.getLocInfo() is CCValAssign::Indirect, we can NOT copy object. Instead we just restore the pointer.
(Flags.isByVal() && VA.getLocInfo() != CCValAssign::Indirect)

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

3774

The hasCopy should just be isByVal with no inversion and the Flags.isByVal() should be removed and replaced with isByVal. isByVal replaced the version in Flags.

Or what Yuanke said might work.

[LiuChen3]

Address the comments

[LuoYuanke]

LGTM

[craig.topper]

LGTM

llvm/lib/Target/X86/X86ISelLowering.h

private:
     SDValue LowerMemArgument(SDValue Chain, CallingConv::ID CallConv,
                              const SmallVectorImpl<ISD::InputArg> &ArgInfo,
                              const SDLoc &dl, SelectionDAG &DAG,
                              const CCValAssign &VA, MachineFrameInfo &MFI,
                              unsigned i) const;
     SDValue LowerMemOpCallTo(SDValue Chain, SDValue StackPtr, SDValue Arg,
                              const SDLoc &dl, SelectionDAG &DAG,
                              const CCValAssign &VA,
-                             ISD::ArgFlagsTy Flags) const;
+                             ISD::ArgFlagsTy Flags, bool hasCopy) const;
 
     // Call lowering helpers.
 
     /// Check whether the call is eligible for tail call optimization. Targets
     /// that want to do tail call optimization should implement this function.
     bool IsEligibleForTailCallOptimization(SDValue Callee,
                                            CallingConv::ID CalleeCC,
                                            bool isVarArg,

llvm/lib/Target/X86/X86ISelLowering.cpp

SDValue X86TargetLowering::LowerFormalArguments(
 
   return Chain;
 }
 
 SDValue X86TargetLowering::LowerMemOpCallTo(SDValue Chain, SDValue StackPtr,
                                             SDValue Arg, const SDLoc &dl,
                                             SelectionDAG &DAG,
                                             const CCValAssign &VA,
-                                            ISD::ArgFlagsTy Flags) const {
+                                            ISD::ArgFlagsTy Flags,
+                                            bool hasCopy) const {
   unsigned LocMemOffset = VA.getLocMemOffset();
   SDValue PtrOff = DAG.getIntPtrConstant(LocMemOffset, dl);
   PtrOff = DAG.getNode(ISD::ADD, dl, getPointerTy(DAG.getDataLayout()),
                        StackPtr, PtrOff);
-  if (Flags.isByVal())
+  // If the argument already has a copy on the stack, we do not need to
+  // creating a temporary stack slot, again.
+  if (Flags.isByVal() && !hasCopy)
     return CreateCopyOfByValArgument(Arg, PtrOff, Chain, Flags, DAG, dl);
 
   return DAG.getStore(
       Chain, dl, Arg, PtrOff,
       MachinePointerInfo::getStack(DAG.getMachineFunction(), LocMemOffset));
 }
 
 /// Emit a load of return address if tail call
case CCValAssign::Indirect: {
             std::max(Align(16), Flags.getNonZeroByValAlign()), false);
         SDValue StackSlot =
             DAG.getFrameIndex(FrameIdx, getPointerTy(DAG.getDataLayout()));
         Chain =
             CreateCopyOfByValArgument(Arg, StackSlot, Chain, Flags, DAG, dl);
         // From now on treat this as a regular pointer
         Arg = StackSlot;
         isByVal = false;
       } else {
         // Store the argument.
         SDValue SpillSlot = DAG.CreateStackTemporary(VA.getValVT());
         int FI = cast<FrameIndexSDNode>(SpillSlot)->getIndex();
         Chain = DAG.getStore(
             Chain, dl, Arg, SpillSlot,
             MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI));
         Arg = SpillSlot;
       }
if (VA.needsCustom()) {
           RegsToPass.push_back(std::make_pair(ShadowReg, Arg));
       }
     } else if (!IsSibcall && (!isTailCall || isByVal)) {
       assert(VA.isMemLoc());
       if (!StackPtr.getNode())
         StackPtr = DAG.getCopyFromReg(Chain, dl, RegInfo->getStackRegister(),
                                       getPointerTy(DAG.getDataLayout()));
       MemOpChains.push_back(LowerMemOpCallTo(Chain, StackPtr, Arg,
-                                             dl, DAG, VA, Flags));
+                                             dl, DAG, VA, Flags, !isByVal));
     }
   }
 
   if (!MemOpChains.empty())
     Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, MemOpChains);
 
   if (Subtarget.isPICStyleGOT()) {
     // ELF / PIC requires GOT in the EBX register before function calls via PLT

llvm/test/CodeGen/X86/win64-byval.ll

 ; CHECK: movq (%rcx), %rax
 ; CHECK: movq 8(%rcx), %rcx
 ; CHECK: movq %rcx, 40(%rsp)
 ; CHECK: movq %rax, 32(%rsp)
 ; CHECK: leaq 32(%rsp), %rcx
     call void @foo({ float, double }* byval %arg)
     ret void
 }
+
+declare void @foo2({ float, double }* byval, { float, double }* byval, { float, double }* byval, { float, double }* byval, { float, double }* byval, i64 %f)
+@data = external constant { float, double }
+
+define void @test() {
+; CHECK-LABEL: @test
+; CHECK:      movq    (%rax), %rcx
+; CHECK-NEXT: movq    8(%rax), %rax
+; CHECK-NEXT: movq    %rax, 120(%rsp)
+; CHECK-NEXT: movq    %rcx, 112(%rsp)
+; CHECK-NEXT: movq    %rcx, 96(%rsp)
+; CHECK-NEXT: movq    %rax, 104(%rsp)
+; CHECK-NEXT: movq    %rcx, 80(%rsp)
+; CHECK-NEXT: movq    %rax, 88(%rsp)
+; CHECK-NEXT: movq    %rcx, 64(%rsp)
+; CHECK-NEXT: movq    %rax, 72(%rsp)
+; CHECK-NEXT: movq    %rax, 56(%rsp)
+; CHECK-NEXT: movq    %rcx, 48(%rsp)
+; CHECK-NEXT: leaq    48(%rsp), %rax
+; CHECK-NEXT: movq    %rax, 32(%rsp)
+; CHECK-NEXT: movq    $10, 40(%rsp)
+; CHECK-NEXT: leaq    112(%rsp), %rcx
+; CHECK-NEXT: leaq    96(%rsp), %rdx
+; CHECK-NEXT: leaq    80(%rsp), %r8
+; CHECK-NEXT: leaq    64(%rsp), %r9
+  call void @foo2({ float, double }* byval @G, { float, double }* byval @G, { float, double }* byval @G, { float, double }* byval @G, { float, double }* byval @G, i64 10)
+  ret void
+}