[llvm] r208637 - Try to fix an SDAG dependence issue with sret

[Reid Kleckner]

Author: rnk
Date: Mon May 12 17:01:27 2014
New Revision: 208637

URL: http://llvm.org/viewvc/llvm-project?rev=208637&view=rev
Log:
Try to fix an SDAG dependence issue with sret

r208453 added support for having sret on the second parameter.  In that
change, the code for copying sret into a virtual register was hoisted
into the loop that lowers formal parameters.  This caused a "Wrong
topological sorting" assertion failure during scheduling when a
parameter is passed in memory.  This change undoes that by creating a
second loop that deals with sret.

I'm worried that this fix is incomplete.  I don't fully understand the
dependence issues.  However, with this change we produce the same DAGs
we used to produce, so if they are broken, they are just as broken as
they have always been.

Added:
    llvm/trunk/test/CodeGen/X86/x86-64-sret-return-2.ll
Modified:
    llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp
    llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

Modified: llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp?rev=208637&r1=208636&r2=208637&view=diff
==============================================================================
--- llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp (original)
+++ llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp Mon May 12 17:01:27 2014
@@ -2660,20 +2660,22 @@ MipsTargetLowering::LowerFormalArguments
       InVals.push_back(Load);
       OutChains.push_back(Load.getValue(1));
     }
+  }
 
+  for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
     // The mips ABIs for returning structs by value requires that we copy
     // the sret argument into $v0 for the return. Save the argument into
     // a virtual register so that we can access it from the return points.
-    if (Flags.isSRet()) {
+    if (Ins[i].Flags.isSRet()) {
       unsigned Reg = MipsFI->getSRetReturnReg();
       if (!Reg) {
         Reg = MF.getRegInfo().createVirtualRegister(
             getRegClassFor(isN64() ? MVT::i64 : MVT::i32));
         MipsFI->setSRetReturnReg(Reg);
       }
-      SDValue Copy =
-          DAG.getCopyToReg(DAG.getEntryNode(), DL, Reg, InVals.back());
+      SDValue Copy = DAG.getCopyToReg(DAG.getEntryNode(), DL, Reg, InVals[i]);
       Chain = DAG.getNode(ISD::TokenFactor, DL, MVT::Other, Copy, Chain);
+      break;
     }
   }
 

Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=208637&r1=208636&r2=208637&view=diff
==============================================================================
--- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
+++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Mon May 12 17:01:27 2014
@@ -2299,23 +2299,26 @@ X86TargetLowering::LowerFormalArguments(
                              MachinePointerInfo(), false, false, false, 0);
 
     InVals.push_back(ArgValue);
+  }
 
-    // The x86-64 ABIs require that for returning structs by value we copy
-    // the sret argument into %rax/%eax (depending on ABI) for the return.
-    // Win32 requires us to put the sret argument to %eax as well.
-    // Save the argument into a virtual register so that we can access it
-    // from the return points.
-    if (Ins[i].Flags.isSRet() &&
-        (Subtarget->is64Bit() || Subtarget->isTargetKnownWindowsMSVC())) {
-      unsigned Reg = FuncInfo->getSRetReturnReg();
-      if (!Reg) {
-        MVT PtrTy = getPointerTy();
-        Reg = MF.getRegInfo().createVirtualRegister(getRegClassFor(PtrTy));
-        FuncInfo->setSRetReturnReg(Reg);
+  if (Subtarget->is64Bit() || Subtarget->isTargetKnownWindowsMSVC()) {
+    for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+      // The x86-64 ABIs require that for returning structs by value we copy
+      // the sret argument into %rax/%eax (depending on ABI) for the return.
+      // Win32 requires us to put the sret argument to %eax as well.
+      // Save the argument into a virtual register so that we can access it
+      // from the return points.
+      if (Ins[i].Flags.isSRet()) {
+        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[i]);
+        Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Copy, Chain);
+        break;
       }
-      SDValue Copy =
-          DAG.getCopyToReg(DAG.getEntryNode(), dl, Reg, InVals.back());
-      Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Copy, Chain);
     }
   }
 

Added: llvm/trunk/test/CodeGen/X86/x86-64-sret-return-2.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/x86-64-sret-return-2.ll?rev=208637&view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/X86/x86-64-sret-return-2.ll (added)
+++ llvm/trunk/test/CodeGen/X86/x86-64-sret-return-2.ll Mon May 12 17:01:27 2014
@@ -0,0 +1,18 @@
+; RUN: llc -mtriple=x86_64-apple-darwin8 < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-pc-linux < %s | FileCheck %s
+
+; FIXME: x32 doesn't know how to select this.  This isn't a regression, it never
+; worked.
+; RUNX: llc -mtriple=x86_64-pc-linux-gnux32 < %s | FileCheck -check-prefix=X32ABI %s
+
+; This used to crash due to topological sorting issues in selection DAG.
+define void @foo(i32* sret %agg.result, i32, i32, i32, i32, i32, void (i32)* %pred) {
+entry:
+  call void %pred(i32 undef)
+  ret void
+
+; CHECK-LABEL: foo:
+; CHECK: callq
+; CHECK: movq {{.*}}, %rax
+; CHECK: ret
+}