DeadArgumentElimination: keep return value on functions that have a live argument with the 'returned' attribute (rather than generate invalid IR); however, if both can be eliminated, both will be

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@185290 91177308-0d34-0410-b5e6-96231b3b80d8

2 files changed, 115 insertions, 28 deletions

diff --git a/lib/Transforms/IPO/DeadArgumentElimination.cpp b/lib/Transforms/IPO/DeadArgumentElimination.cpp
index 2e27ae0..6ee6162 100644
--- a/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ b/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -716,10 +716,42 @@ bool DAE::RemoveDeadStuffFromFunction(Function *F) {
   FunctionType *FTy = F->getFunctionType();
   std::vector<Type*> Params;
 
+  // Keep track of if we have a live 'returned' argument
+  bool HasLiveReturnedArg = false;
+
   // Set up to build a new list of parameter attributes.
   SmallVector<AttributeSet, 8> AttributesVec;
   const AttributeSet &PAL = F->getAttributes();
 
+  // Remember which arguments are still alive.
+  SmallVector<bool, 10> ArgAlive(FTy->getNumParams(), false);
+  // Construct the new parameter list from non-dead arguments. Also construct
+  // a new set of parameter attributes to correspond. Skip the first parameter
+  // attribute, since that belongs to the return value.
+  unsigned i = 0;
+  for (Function::arg_iterator I = F->arg_begin(), E = F->arg_end();
+       I != E; ++I, ++i) {
+    RetOrArg Arg = CreateArg(F, i);
+    if (LiveValues.erase(Arg)) {
+      Params.push_back(I->getType());
+      ArgAlive[i] = true;
+
+      // Get the original parameter attributes (skipping the first one, that is
+      // for the return value.
+      if (PAL.hasAttributes(i + 1)) {
+        AttrBuilder B(PAL, i + 1);
+        if (B.contains(Attribute::Returned))
+          HasLiveReturnedArg = true;
+        AttributesVec.
+          push_back(AttributeSet::get(F->getContext(), Params.size(), B));
+      }
+    } else {
+      ++NumArgumentsEliminated;
+      DEBUG(dbgs() << "DAE - Removing argument " << i << " (" << I->getName()
+            << ") from " << F->getName() << "\n");
+    }
+  }
+
   // Find out the new return value.
   Type *RetTy = FTy->getReturnType();
   Type *NRetTy = NULL;
@@ -728,7 +760,27 @@ bool DAE::RemoveDeadStuffFromFunction(Function *F) {
   // -1 means unused, other numbers are the new index
   SmallVector<int, 5> NewRetIdxs(RetCount, -1);
   std::vector<Type*> RetTypes;
-  if (RetTy->isVoidTy()) {
+
+  // If there is a function with a live 'returned' argument but a dead return
+  // value, then there are two possible actions:
+  // 1) Eliminate the return value and take off the 'returned' attribute on the
+  //    argument.
+  // 2) Retain the 'returned' attribute and treat the return value (but not the
+  //    entire function) as live so that it is not eliminated.
+  // 
+  // It's not clear in the general case which option is more profitable because,
+  // even in the absence of explicit uses of the return value, code generation
+  // is free to use the 'returned' attribute to do things like eliding
+  // save/restores of registers across calls. Whether or not this happens is
+  // target and ABI-specific as well as depending on the amount of register
+  // pressure, so there's no good way for an IR-level pass to figure this out.
+  //
+  // Fortunately, the only places where 'returned' is currently generated by
+  // the FE are places where 'returned' is basically free and almost always a
+  // performance win, so the second option can just be used always for now.
+  //
+  // This should be revisited if 'returned' is ever applied more liberally.
+  if (RetTy->isVoidTy() || HasLiveReturnedArg) {
     NRetTy = RetTy;
   } else {
     StructType *STy = dyn_cast<StructType>(RetTy);
@@ -798,33 +850,6 @@ bool DAE::RemoveDeadStuffFromFunction(Function *F) {
   if (RAttrs.hasAttributes(AttributeSet::ReturnIndex))
     AttributesVec.push_back(AttributeSet::get(NRetTy->getContext(), RAttrs));
 
-  // Remember which arguments are still alive.
-  SmallVector<bool, 10> ArgAlive(FTy->getNumParams(), false);
-  // Construct the new parameter list from non-dead arguments. Also construct
-  // a new set of parameter attributes to correspond. Skip the first parameter
-  // attribute, since that belongs to the return value.
-  unsigned i = 0;
-  for (Function::arg_iterator I = F->arg_begin(), E = F->arg_end();
-       I != E; ++I, ++i) {
-    RetOrArg Arg = CreateArg(F, i);
-    if (LiveValues.erase(Arg)) {
-      Params.push_back(I->getType());
-      ArgAlive[i] = true;
-
-      // Get the original parameter attributes (skipping the first one, that is
-      // for the return value.
-      if (PAL.hasAttributes(i + 1)) {
-        AttrBuilder B(PAL, i + 1);
-        AttributesVec.
-          push_back(AttributeSet::get(F->getContext(), Params.size(), B));
-      }
-    } else {
-      ++NumArgumentsEliminated;
-      DEBUG(dbgs() << "DAE - Removing argument " << i << " (" << I->getName()
-            << ") from " << F->getName() << "\n");
-    }
-  }
-
   if (PAL.hasAttributes(AttributeSet::FunctionIndex))
     AttributesVec.push_back(AttributeSet::get(F->getContext(),
                                               PAL.getFnAttributes()));
@@ -885,6 +910,13 @@ bool DAE::RemoveDeadStuffFromFunction(Function *F) {
         // Get original parameter attributes, but skip return attributes.
         if (CallPAL.hasAttributes(i + 1)) {
           AttrBuilder B(CallPAL, i + 1);
+          // If the return type has changed, then get rid of 'returned' on the
+          // call site. The alternative is to make all 'returned' attributes on
+          // call sites keep the return value alive just like 'returned'
+          // attributes on function declaration but it's less clearly a win
+          // and this is not an expected case anyway
+          if (NRetTy != RetTy && B.contains(Attribute::Returned))
+            B.removeAttribute(Attribute::Returned);
           AttributesVec.
             push_back(AttributeSet::get(F->getContext(), Args.size(), B));
         }
diff --git a/test/Transforms/DeadArgElim/returned.ll b/test/Transforms/DeadArgElim/returned.ll
new file mode 100644
index 0000000..c4d3ec7
--- /dev/null
+++ b/test/Transforms/DeadArgElim/returned.ll
@@ -0,0 +1,55 @@
+; RUN: opt < %s -deadargelim -S | FileCheck %s
+
+%Ty = type { i32, i32 }
+
+; sanity check that the argument and return value are both dead
+; CHECK: define internal void @test1()
+
+define internal %Ty* @test1(%Ty* %this) {
+  ret %Ty* %this
+}
+
+; do not keep alive the return value of a function with a dead 'returned' argument
+; CHECK: define internal void @test2()
+
+define internal %Ty* @test2(%Ty* returned %this) {
+  ret %Ty* %this
+}
+
+; dummy to keep 'this' alive
+@dummy = global %Ty* null 
+
+; sanity check that return value is dead
+; CHECK: define internal void @test3(%Ty* %this)
+
+define internal %Ty* @test3(%Ty* %this) {
+  store volatile %Ty* %this, %Ty** @dummy
+  ret %Ty* %this
+}
+
+; keep alive return value of a function if the 'returned' argument is live
+; CHECK: define internal %Ty* @test4(%Ty* returned %this)
+
+define internal %Ty* @test4(%Ty* returned %this) {
+  store volatile %Ty* %this, %Ty** @dummy
+  ret %Ty* %this
+}
+
+; don't do this if 'returned' is on the call site...
+; CHECK: define internal void @test5(%Ty* %this)
+
+define internal %Ty* @test5(%Ty* %this) {
+  store volatile %Ty* %this, %Ty** @dummy
+  ret %Ty* %this
+}
+
+define %Ty* @caller(%Ty* %this) {
+  %1 = call %Ty* @test1(%Ty* %this)
+  %2 = call %Ty* @test2(%Ty* %this)
+  %3 = call %Ty* @test3(%Ty* %this)
+  %4 = call %Ty* @test4(%Ty* %this)
+; ...instead, drop 'returned' form the call site
+; CHECK: call void @test5(%Ty* %this)
+  %5 = call %Ty* @test5(%Ty* returned %this)
+  ret %Ty* %this
+}