[CodeGen] Reuse stack space from unused function results (with more accurate unused result detection)

[lenykholodov]

Summary

This patch fixes issues with unused result detection which were found in patch http://reviews.llvm.org/D9743.

For detection of unused result the method CodeGenFunction::EmitCall uses similar approach to Sema::DiagnoseUnusedExprResult. Sema::DiagnoseUnusedExprResult uses underlying method Expr::isUnusedResultAWarning(const Expr *&WarnExpr, SourceLocation &Loc, SourceRange &R1, SourceRange &R2, ASTContext &Ctx) for accurate detection. This method has few drawbacks for using outside of Sema class:

• it checks warn_unused_result attribute which in general case is not set;
• it modifies several parameters which are not required for unused result checking.

In general case, we want to see smth similar to this method but without warn_unused_attribute checking and without modifiable parameters. The most expected solution in this case may be to provide Expr::isUnusedResult method which the method isUnusedResultAWarning method should use. However, this can't be easily done because in this case all context information for diagnostic should be detected repeatedly inside isUnusedResultAWarning method. Same for warn_unused_attribute. Another solution might be to duplicate logic of isUnusedResultAWarning inside inUnusedResult, but this duplication is too big (few hundred LOC). Moreover, all changes inside isUnusedResultAWarning should be duplicated inside isUnusedResult too. So this doesn't look acceptable.

This patch provides solution which uses isUnusedResultAWarning as an underlying method for isUnusedResult. It extends the list of parameters of isUnusedResultAWarning with extra parameter alwaysWarnIfUnused (default value is false). Based on the isUnusedResult method patch detects unused result during the code generation.

[rnk]

I don't think this is the right approach. The code that you're reusing is in theory checking the same thing as codegen, but the implementations are distant, unrelated, and I suspect out of sync. I think this will probably be easier to handle in AggExprEmitter, which has more accurate information about its destinations.

You might also want to use the cleanup mechanism instead of trying to manually insert IR after emitting the function call. We should probably add a helper for pushing a CallLifetimeEnd cleanup that takes the slot address and size.

[lenykholodov]

Reid, first of all thank you very much for your comments. I've updated the implementation. Now I make decision about result usage inside CodeGenFunction::EmitAggExpr method. For an unused result I have incoming expression with type CallExpr. But for the used result I have two calls of EmitAggExpr method second of which has incoming expression with type MaterializeTemporaryExpr. So I decided to mark result as used in case of incoming MaterializeTemporaryExpr expression and to mark result as unused in other cases.

In the current implementation I don't use automatically cleanup mechanism for CallLifetimeEnd. I want to prepare it in separate patch.

[rnk]

Can you add some negative lifetime tests? These would be corner case situations like the original Twine bug where we thought the call result was unused but it's actually consumed by someone.

lib/CodeGen/CGExprAgg.cpp

1398 ↗

(On Diff #26798)

This condition doesn't seem right. You can Slot.isIgnored(), which should do the trick, though. :)

[lenykholodov]

Changes:

• Slot.isIgnored() is used for detection of unused result;
• test case 'large_combiner_test()' has been added; it doesn't work with previous patch (D9743) and works now; after codegen this test has no lifetime scope, and clang detects results of internal calls as used.

[rnk]

lib/CodeGen/CGCall.h

158–159 ↗

(On Diff #26977)

There's another low alignment bit here, you should really fold it in here to keep this thing pointer-sized. You can either nest PointerIntPair<> twice or use PointerIntPair<llvm::Value *, 2, unsigned> Value and then do bitwise arithmetic in the helpers.

lib/CodeGen/CGExprAgg.cpp

65 ↗

(On Diff #26977)

You can follow the convention for Dest and name the parameter after the member with the same case. It's correct.

[lenykholodov]

• ReturnValueSlot class size optimization using packed to PointerIntPair<> flags;
• minor changes.

[rnk]

lgtm, let's give it a shot

cfe/trunk/lib/CodeGen/CGCall.h

namespace CodeGen {
   /// of parameters to a function. The decl must be either a
   /// ParmVarDecl or ImplicitParamDecl.
   class FunctionArgList : public SmallVector<const VarDecl*, 16> {
   };
 
   /// ReturnValueSlot - Contains the address where the return value of a 
   /// function can be stored, and whether the address is volatile or not.
   class ReturnValueSlot {
-    llvm::PointerIntPair<llvm::Value *, 1, bool> Value;
+    llvm::PointerIntPair<llvm::Value *, 2, unsigned int> Value;
+
+    // Return value slot flags
+    enum Flags {
+      IS_VOLATILE = 0x1,
+      IS_UNUSED = 0x2,
+    };
 
   public:
     ReturnValueSlot() {}
-    ReturnValueSlot(llvm::Value *Value, bool IsVolatile)
-      : Value(Value, IsVolatile) {}
+    ReturnValueSlot(llvm::Value *Value, bool IsVolatile, bool IsUnused = false)
+      : Value(Value,
+              (IsVolatile ? IS_VOLATILE : 0) | (IsUnused ? IS_UNUSED : 0)) {}
 
     bool isNull() const { return !getValue(); }
-    
-    bool isVolatile() const { return Value.getInt(); }
+    bool isVolatile() const { return Value.getInt() & IS_VOLATILE; }
     llvm::Value *getValue() const { return Value.getPointer(); }
+    bool isUnused() const { return Value.getInt() & IS_UNUSED; }
   };
   
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

cfe/trunk/lib/CodeGen/CGCall.cpp

RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
   }
 
   ClangToLLVMArgMapping IRFunctionArgs(CGM.getContext(), CallInfo);
   SmallVector<llvm::Value *, 16> IRCallArgs(IRFunctionArgs.totalIRArgs());
 
   // If the call returns a temporary with struct return, create a temporary
   // alloca to hold the result, unless one is given to us.
   llvm::Value *SRetPtr = nullptr;
+  size_t UnusedReturnSize = 0;
   if (RetAI.isIndirect() || RetAI.isInAlloca()) {
     SRetPtr = ReturnValue.getValue();
-    if (!SRetPtr)
+    if (!SRetPtr) {
       SRetPtr = CreateMemTemp(RetTy);
+      if (HaveInsertPoint() && ReturnValue.isUnused()) {
+        uint64_t size =
+            CGM.getDataLayout().getTypeAllocSize(ConvertTypeForMem(RetTy));
+        if (EmitLifetimeStart(size, SRetPtr))
+          UnusedReturnSize = size;
+      }
+    }
     if (IRFunctionArgs.hasSRetArg()) {
       IRCallArgs[IRFunctionArgs.getSRetArgNo()] = SRetPtr;
     } else {
       llvm::Value *Addr =
           Builder.CreateStructGEP(ArgMemory->getAllocatedType(), ArgMemory,
                                   RetAI.getInAllocaFieldIndex());
       Builder.CreateStore(SRetPtr, Addr);
     }
#endif
   // optimizer it can aggressively ignore unwind edges.
   if (CGM.getLangOpts().ObjCAutoRefCount)
     AddObjCARCExceptionMetadata(CS.getInstruction());
 
   // If the call doesn't return, finish the basic block and clear the
   // insertion point; this allows the rest of IRgen to discard
   // unreachable code.
   if (CS.doesNotReturn()) {
+    if (UnusedReturnSize)
+      EmitLifetimeEnd(llvm::ConstantInt::get(Int64Ty, UnusedReturnSize),
+                      SRetPtr);
+
     Builder.CreateUnreachable();
     Builder.ClearInsertionPoint();
 
     // FIXME: For now, emit a dummy basic block because expr emitters in
     // generally are not ready to handle emitting expressions at unreachable
     // points.
     EnsureInsertPoint();
 
#endif
 
   // The stack cleanup for inalloca arguments has to run out of the normal
   // lexical order, so deactivate it and run it manually here.
   CallArgs.freeArgumentMemory(*this);
 
   RValue Ret = [&] {
     switch (RetAI.getKind()) {
     case ABIArgInfo::InAlloca:
-    case ABIArgInfo::Indirect:
-      return convertTempToRValue(SRetPtr, RetTy, SourceLocation());
+    case ABIArgInfo::Indirect: {
+      RValue ret = convertTempToRValue(SRetPtr, RetTy, SourceLocation());
+      if (UnusedReturnSize)
+        EmitLifetimeEnd(llvm::ConstantInt::get(Int64Ty, UnusedReturnSize),
+                        SRetPtr);
+      return ret;
+    }
 
     case ABIArgInfo::Ignore:
       // If we are ignoring an argument that had a result, make sure to
       // construct the appropriate return value for our caller.
       return GetUndefRValue(RetTy);
 
     case ABIArgInfo::Extend:
     case ABIArgInfo::Direct: {

cfe/trunk/lib/CodeGen/CGExprAgg.cpp

 //                        Aggregate Expression Emitter
 //===----------------------------------------------------------------------===//
 
 namespace  {
 class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
   CodeGenFunction &CGF;
   CGBuilderTy &Builder;
   AggValueSlot Dest;
+  bool IsResultUnused;
 
   /// We want to use 'dest' as the return slot except under two
   /// conditions:
   ///   - The destination slot requires garbage collection, so we
   ///     need to use the GC API.
   ///   - The destination slot is potentially aliased.
   bool shouldUseDestForReturnSlot() const {
     return !(Dest.requiresGCollection() || Dest.isPotentiallyAliased());
   }
 
   ReturnValueSlot getReturnValueSlot() const {
     if (!shouldUseDestForReturnSlot())
       return ReturnValueSlot();
 
-    return ReturnValueSlot(Dest.getAddr(), Dest.isVolatile());
+    return ReturnValueSlot(Dest.getAddr(), Dest.isVolatile(), IsResultUnused);
   }
 
   AggValueSlot EnsureSlot(QualType T) {
     if (!Dest.isIgnored()) return Dest;
     return CGF.CreateAggTemp(T, "agg.tmp.ensured");
   }
   void EnsureDest(QualType T) {
     if (!Dest.isIgnored()) return;
     Dest = CGF.CreateAggTemp(T, "agg.tmp.ensured");
   }
 
 public:
-  AggExprEmitter(CodeGenFunction &cgf, AggValueSlot Dest)
-    : CGF(cgf), Builder(CGF.Builder), Dest(Dest) {
-  }
+  AggExprEmitter(CodeGenFunction &cgf, AggValueSlot Dest, bool IsResultUnused)
+    : CGF(cgf), Builder(CGF.Builder), Dest(Dest),
+    IsResultUnused(IsResultUnused) { }
 
   //===--------------------------------------------------------------------===//
   //                               Utilities
   //===--------------------------------------------------------------------===//
 
   /// EmitAggLoadOfLValue - Given an expression with aggregate type that
   /// represents a value lvalue, this method emits the address of the lvalue,
   /// then loads the result into DestPtr.
void CodeGenFunction::EmitAggExpr(const Expr *E, AggValueSlot Slot) {
   assert(E && hasAggregateEvaluationKind(E->getType()) &&
          "Invalid aggregate expression to emit");
   assert((Slot.getAddr() != nullptr || Slot.isIgnored()) &&
          "slot has bits but no address");
 
   // Optimize the slot if possible.
   CheckAggExprForMemSetUse(Slot, E, *this);
  
-  AggExprEmitter(*this, Slot).Visit(const_cast<Expr*>(E));
+  AggExprEmitter(*this, Slot, Slot.isIgnored()).Visit(const_cast<Expr*>(E));
 }
 
 LValue CodeGenFunction::EmitAggExprToLValue(const Expr *E) {
   assert(hasAggregateEvaluationKind(E->getType()) && "Invalid argument!");
   llvm::Value *Temp = CreateMemTemp(E->getType());
   LValue LV = MakeAddrLValue(Temp, E->getType());
   EmitAggExpr(E, AggValueSlot::forLValue(LV, AggValueSlot::IsNotDestructed,
                                          AggValueSlot::DoesNotNeedGCBarriers,

cfe/trunk/test/CodeGenCXX/stack-reuse-miscompile.cpp

+// RUN: %clang -S -emit-llvm -O1 -mllvm -disable-llvm-optzns -S %s -o - | FileCheck %s
+
+// This test should not to generate llvm.lifetime.start/llvm.lifetime.end for
+// f function because all temporary objects in this function are used for the
+// final result
+
+class S {
+  char *ptr;
+  unsigned int len;
+};
+
+class T {
+  S left;
+  S right;
+
+public:
+  T(const char s[]);
+  T(S);
+
+  T concat(const T &Suffix) const;
+  const char * str() const;
+};
+
+const char * f(S s)
+{
+// CHECK: %1 = alloca %class.T, align 4
+// CHECK: %2 = alloca %class.T, align 4
+// CHECK: %3 = alloca %class.S, align 4
+// CHECK: %4 = alloca %class.T, align 4
+// CHECK: %5 = call x86_thiscallcc %class.T* @"\01??0T@@QAE@QBD@Z"
+// CHECK: %6 = bitcast %class.S* %3 to i8*
+// CHECK: %7 = bitcast %class.S* %s to i8*
+// CHECK: call void @llvm.memcpy.p0i8.p0i8.i32
+// CHECK: %8 = call x86_thiscallcc %class.T* @"\01??0T@@QAE@VS@@@Z"
+// CHECK: call x86_thiscallcc void @"\01?concat@T@@QBE?AV1@ABV1@@Z"
+// CHECK: %9 = call x86_thiscallcc i8* @"\01?str@T@@QBEPBDXZ"(%class.T* %4)
+
+  return T("[").concat(T(s)).str();
+}

cfe/trunk/test/CodeGenCXX/stack-reuse.cpp

+// RUN: %clang -target armv7l-unknown-linux-gnueabihf -S %s -o - -emit-llvm -O1 -disable-llvm-optzns | FileCheck %s
+
+// Stack should be reused when possible, no need to allocate two separate slots
+// if they have disjoint lifetime.
+
+// Sizes of objects are related to previously existed threshold of 32.  In case
+// of S_large stack size is rounded to 40 bytes.
+
+// 32B
+struct S_small {
+  int a[8];
+};
+
+// 36B
+struct S_large {
+  int a[9];
+};
+
+// Helper class for lifetime scope absence testing
+struct Combiner {
+  S_large a, b;
+
+  Combiner(S_large);
+  Combiner f();  
+};
+
+extern S_small foo_small();
+extern S_large foo_large();
+extern void bar_small(S_small*);
+extern void bar_large(S_large*);
+
+// Prevent mangling of function names.
+extern "C" {
+
+void small_rvoed_unnamed_temporary_object() {
+// CHECK-LABEL: define void @small_rvoed_unnamed_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+
+  foo_small();
+  foo_small();
+}
+
+void large_rvoed_unnamed_temporary_object() {
+// CHECK-LABEL: define void @large_rvoed_unnamed_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+
+  foo_large();
+  foo_large();
+}
+
+void small_rvoed_named_temporary_object() {
+// CHECK-LABEL: define void @small_rvoed_named_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+
+  {
+    S_small s = foo_small();
+  }
+  {
+    S_small s = foo_small();
+  }
+}
+
+void large_rvoed_named_temporary_object() {
+// CHECK-LABEL: define void @large_rvoed_named_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+
+  {
+    S_large s = foo_large();
+  }
+  {
+    S_large s = foo_large();
+  }
+}
+
+void small_auto_object() {
+// CHECK-LABEL: define void @small_auto_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_smallP7S_small
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_smallP7S_small
+// CHECK: call void @llvm.lifetime.end
+
+  {
+    S_small s;
+    bar_small(&s);
+  }
+  {
+    S_small s;
+    bar_small(&s);
+  }
+}
+
+void large_auto_object() {
+// CHECK-LABEL: define void @large_auto_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_largeP7S_large
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_largeP7S_large
+// CHECK: call void @llvm.lifetime.end
+
+  {
+    S_large s;
+    bar_large(&s);
+  }
+  {
+    S_large s;
+    bar_large(&s);
+  }
+}
+
+int large_combiner_test(S_large s) {
+// CHECK-LABEL: define i32 @large_combiner_test
+// CHECK: %1 = alloca %struct.Combiner
+// CHECK: %2 = alloca %struct.Combiner
+// CHECK: %3 = call %struct.Combiner* @_ZN8CombinerC1E7S_large(%struct.Combiner* %1, [9 x i32] %s.coerce)
+// CHECK: call void @_ZN8Combiner1fEv(%struct.Combiner* sret %2, %struct.Combiner* %1)
+// CHECK: %4 = getelementptr inbounds %struct.Combiner, %struct.Combiner* %2, i32 0, i32 0, i32 0, i32 0
+// CHECK: %5 = load i32, i32* %4
+// CHECK: ret i32 %5
+
+  return Combiner(s).f().a.a[0];
+}
+
+}