Allow sret on the second parameter as well as the first

[rnk]

Summary

MSVC always places the implicit sret parameter after the implicit this
parameter of instance methods. We used to handle this for
x86_thiscallcc by allocating the sret parameter on the stack and leaving
the this pointer in ecx, but that doesn't handle alternative calling
conventions like cdecl, stdcall, fastcall, or the win64 convention.

Instead, change the verifier to allow sret on the second parameter.

This also requires changing the X86 backend to return the argument with
the sret parameter, instead of assuming that the sret parameter comes
first.

[rnk]

Erp, this was two patches smashed together.

[rnk]

remove silly patch

[majnemer]

LGTM

lib/Target/X86/X86ISelLowering.cpp

2310

We should assert I != E.

[• rafael]

lib/Target/X86/X86ISelLowering.cpp

2307

Why the loop? If the rest of the code assumes that it is the first or second, it seems better to check only those too, or at least assert that I is 0 or 1.

[rnk]

• Fix issue where the first param is split and add tests

[rnk]

I looked for other backends that return sret parameters and found that mips and sparc do this. Mips can be updated to handle sret on parameters other than the first, but sparc has more assumptions about sret, so I stuck in a report_fatal_error call.

lib/Target/X86/X86ISelLowering.cpp

2307

I realized this loop was wrong anyway if codegen split the first parameter into multiple EVTs, like i64 to 2 i32s on a 32-bit platform. I moved it up into the loop producing InVals and added a test for it.

[rnk]

Closed by commit rL208453 (authored by @rnk).

include/llvm/IR/Function.h

public:
   /// @brief True if this function needs an unwind table.
   bool needsUnwindTableEntry() const {
     return hasUWTable() || !doesNotThrow();
   }
 
   /// @brief Determine if the function returns a structure through first
   /// pointer argument.
   bool hasStructRetAttr() const {
-    return AttributeSets.hasAttribute(1, Attribute::StructRet);
+    return AttributeSets.hasAttribute(1, Attribute::StructRet) ||
+           AttributeSets.hasAttribute(2, Attribute::StructRet);
   }
 
   /// @brief Determine if the parameter does not alias other parameters.
   /// @param n The parameter to check. 1 is the first parameter, 0 is the return
   bool doesNotAlias(unsigned n) const {
     return AttributeSets.hasAttribute(n, Attribute::NoAlias);
   }
   void setDoesNotAlias(unsigned n) {

lib/IR/Verifier.cpp

if (Attrs.hasAttribute(Idx, Attribute::Returned)) {
       Assert1(!SawReturned, "More than one parameter has attribute returned!",
               V);
       Assert1(Ty->canLosslesslyBitCastTo(FT->getReturnType()), "Incompatible "
               "argument and return types for 'returned' attribute", V);
       SawReturned = true;
     }
 
     if (Attrs.hasAttribute(Idx, Attribute::StructRet))
-      Assert1(Idx == 1, "Attribute sret is not on first parameter!", V);
+      Assert1(Idx == 1 || Idx == 2,
+              "Attribute sret is not on first or second parameter!", V);
 
     if (Attrs.hasAttribute(Idx, Attribute::InAlloca)) {
       Assert1(Idx == FT->getNumParams(),
               "inalloca isn't on the last parameter!", V);
     }
   }
 
   if (!Attrs.hasAttributes(AttributeSet::FunctionIndex))

lib/Support/Atomic.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Atomic.h"
 #include "llvm/Config/llvm-config.h"
 
 using namespace llvm;
 
 #if defined(_MSC_VER)
+#include <Intrin.h>
 #include <windows.h>
 #undef MemoryFence
 #endif
 
 #if defined(__GNUC__) || (defined(__IBMCPP__) && __IBMCPP__ >= 1210)
 #define GNU_ATOMICS
 #endif
 

lib/Target/X86/X86ISelLowering.cpp

if (MF.getFunction()->hasStructRetAttr() &&
       (Subtarget->is64Bit() || Subtarget->isTargetKnownWindowsMSVC())) {
     X86MachineFunctionInfo *FuncInfo = MF.getInfo<X86MachineFunctionInfo>();
     unsigned Reg = FuncInfo->getSRetReturnReg();
     if (!Reg) {
       MVT PtrTy = getPointerTy();
       Reg = MF.getRegInfo().createVirtualRegister(getRegClassFor(PtrTy));
       FuncInfo->setSRetReturnReg(Reg);
     }
-    SDValue Copy = DAG.getCopyToReg(DAG.getEntryNode(), dl, Reg, InVals[0]);
+    size_t I = 0, E = MF.getFunction()->arg_size();
+    AttributeSet Attrs = MF.getFunction()->getAttributes();
+    for (; I != E; ++I)
+      if (Attrs.hasAttribute(I + 1, Attribute::StructRet))
+        break;
+    SDValue Copy = DAG.getCopyToReg(DAG.getEntryNode(), dl, Reg, InVals[I]);
     Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Copy, Chain);
   }
 
   unsigned StackSize = CCInfo.getNextStackOffset();
   // Align stack specially for tail calls.
   if (FuncIsMadeTailCallSafe(CallConv,
                              MF.getTarget().Options.GuaranteedTailCallOpt))
     StackSize = GetAlignedArgumentStackSize(StackSize, DAG);

test/CodeGen/X86/win32_sret.ll

 ; CYGWIN-NEXT: leal    8(%esp), %ecx
 ; CYGWIN-NEXT: calll   _test6_g
 
   %tmp = alloca %struct.test6, align 4
   call x86_thiscallcc void @test6_g(%struct.test6* sret %tmp, %struct.test6* %x)
   ret void
 }
 declare x86_thiscallcc void @test6_g(%struct.test6* sret, %struct.test6*)
+
+; Flipping the parameters at the IR level generates the same code.
+%struct.test7 = type { i32, i32, i32 }
+define void @test7_f(%struct.test7* %x) nounwind {
+; WIN32-LABEL: _test7_f:
+; MINGW_X86-LABEL: _test7_f:
+; CYGWIN-LABEL: _test7_f:
+; LINUX-LABEL: test7_f:
+
+; The %x argument is moved to %ecx on all OSs. It will be the this pointer.
+; WIN32:      movl    8(%ebp), %ecx
+; MINGW_X86:  movl    8(%ebp), %ecx
+; CYGWIN:     movl    8(%ebp), %ecx
+
+; The sret pointer is (%esp)
+; WIN32:          leal    8(%esp), %[[REG:e[a-d]x]]
+; WIN32-NEXT:     movl    %[[REG]], (%e{{([a-d]x)|(sp)}})
+; MINGW_X86:      leal    8(%esp), %[[REG:e[a-d]x]]
+; MINGW_X86-NEXT: movl    %[[REG]], (%e{{([a-d]x)|(sp)}})
+; CYGWIN:         leal    8(%esp), %[[REG:e[a-d]x]]
+; CYGWIN-NEXT:    movl    %[[REG]], (%e{{([a-d]x)|(sp)}})
+
+  %tmp = alloca %struct.test7, align 4
+  call x86_thiscallcc void @test7_g(%struct.test7* %x, %struct.test7* sret %tmp)
+  ret void
+}
+declare x86_thiscallcc void @test7_g(%struct.test7*, %struct.test7* sret)