Reapply x2: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[nickdesaulniers]

Summary

This reverts commit b7f4915644844fb9f32e8763922a070f5fe4fd29.

These temporaries are only used in the callee, and their memory can be
reused after the call is complete.

rdar://58552124

Original Author: Erik Pilkington <[e-mail]>

The second time this landed, it had a bug where the lifetimes of
references to the temporaries was cut short prematurely. For now, skip
emitting the lifetime annotations if the callee returns a reference. We
can probably do better than this.

Link: https://github.com/llvm/llvm-project/issues/38157
Link: https://github.com/llvm/llvm-project/issues/41896
Link: https://github.com/llvm/llvm-project/issues/43598
Link: https://github.com/ClangBuiltLinux/linux/issues/39
Link: https://reviews.llvm.org/rGfafc6e4fdf3673dcf557d6c8ae0c0a4bb3184402

[nickdesaulniers]

meant to update another thread

[nickdesaulniers]

• update commit description

[nickdesaulniers]

I hate arcanist!

clang/lib/CodeGen/CGCall.h

public:
   struct CallArgCleanup {
     EHScopeStack::stable_iterator Cleanup;
 
     /// The "is active" insertion point.  This instruction is temporary and
     /// will be removed after insertion.
     llvm::Instruction *IsActiveIP;
   };
 
+  struct EndLifetimeInfo {
+    llvm::Value *Addr;
+    llvm::Value *Size;
+  };
+
   void add(RValue rvalue, QualType type) { push_back(CallArg(rvalue, type)); }
 
   void addUncopiedAggregate(LValue LV, QualType type) {
     push_back(CallArg(LV, type));
   }
 
   /// Add all the arguments from another CallArgList to this one. After doing
   /// this, the old CallArgList retains its list of arguments, but must not
   /// be used to emit a call.
   void addFrom(const CallArgList &other) {
     insert(end(), other.begin(), other.end());
     Writebacks.insert(Writebacks.end(), other.Writebacks.begin(),
                       other.Writebacks.end());
     CleanupsToDeactivate.insert(CleanupsToDeactivate.end(),
                                 other.CleanupsToDeactivate.begin(),
                                 other.CleanupsToDeactivate.end());
+    LifetimeCleanups.insert(LifetimeCleanups.end(),
+                            other.LifetimeCleanups.begin(),
+                            other.LifetimeCleanups.end());
     assert(!(StackBase && other.StackBase) && "can't merge stackbases");
     if (!StackBase)
       StackBase = other.StackBase;
   }
 
   void addWriteback(LValue srcLV, Address temporary, llvm::Value *toUse) {
     Writeback writeback = {srcLV, temporary, toUse};
     Writebacks.push_back(writeback);
public:
   void allocateArgumentMemory(CodeGenFunction &CGF);
   llvm::Instruction *getStackBase() const { return StackBase; }
   void freeArgumentMemory(CodeGenFunction &CGF) const;
 
   /// Returns if we're using an inalloca struct to pass arguments in
   /// memory.
   bool isUsingInAlloca() const { return StackBase; }
 
+  void addLifetimeCleanup(EndLifetimeInfo Info) {
+    LifetimeCleanups.push_back(Info);
+  }
+
+  ArrayRef<EndLifetimeInfo> getLifetimeCleanups() const {
+    return LifetimeCleanups;
+  }
+
 private:
   SmallVector<Writeback, 1> Writebacks;
 
   /// Deactivate these cleanups immediately before making the call.  This
   /// is used to cleanup objects that are owned by the callee once the call
   /// occurs.
   SmallVector<CallArgCleanup, 1> CleanupsToDeactivate;
 
+  /// Lifetime information needed to call llvm.lifetime.end for any temporary
+  /// argument allocas.
+  SmallVector<EndLifetimeInfo, 2> LifetimeCleanups;
+
   /// The stacksave call.  It dominates all of the argument evaluation.
   llvm::CallInst *StackBase = nullptr;
 };
 
 /// FunctionArgList - Type for representing both the decl and type
 /// of parameters to a function. The decl must be either a
 /// ParmVarDecl or ImplicitParamDecl.
 class FunctionArgList : public SmallVector<const VarDecl *, 16> {};

clang/lib/CodeGen/CGCall.cpp

 #include "llvm/IR/AttributeMask.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Support/TypeSize.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <optional>
 using namespace clang;
 using namespace CodeGen;
 
 /***/
 
 unsigned CodeGenTypes::ClangCallConvToLLVMCallConv(CallingConv CC) {
#endif
 
   // Insert a stack save if we're going to need any inalloca args.
   if (hasInAllocaArgs(CGM, ExplicitCC, ArgTypes)) {
     assert(getTarget().getTriple().getArch() == llvm::Triple::x86 &&
            "inalloca only supported on x86");
     Args.allocateArgumentMemory(*this);
   }
 
+  bool ReturnsReference = false;
+  if (const auto *F = dyn_cast_or_null<FunctionDecl>(AC.getDecl()))
+    ReturnsReference = F->getReturnType()->isReferenceType();
+
   // Evaluate each argument in the appropriate order.
   size_t CallArgsStart = Args.size();
   for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
     unsigned Idx = LeftToRight ? I : E - I - 1;
     CallExpr::const_arg_iterator Arg = ArgRange.begin() + Idx;
     unsigned InitialArgSize = Args.size();
     // If *Arg is an ObjCIndirectCopyRestoreExpr, check that either the types of
     // the argument and parameter match or the objc method is parameterized.
     assert((!isa<ObjCIndirectCopyRestoreExpr>(*Arg) ||
             getContext().hasSameUnqualifiedType((*Arg)->getType(),
                                                 ArgTypes[Idx]) ||
             (isa<ObjCMethodDecl>(AC.getDecl()) &&
              isObjCMethodWithTypeParams(cast<ObjCMethodDecl>(AC.getDecl())))) &&
            "Argument and parameter types don't match");
-    EmitCallArg(Args, *Arg, ArgTypes[Idx]);
+    EmitCallArg(Args, *Arg, ArgTypes[Idx], ReturnsReference);
     // In particular, we depend on it being the last arg in Args, and the
     // objectsize bits depend on there only being one arg if !LeftToRight.
     assert(InitialArgSize + 1 == Args.size() &&
            "The code below depends on only adding one arg per EmitCallArg");
     (void)InitialArgSize;
     // Since pointer argument are never emitted as LValue, it is safe to emit
     // non-null argument check for r-value only.
     if (!Args.back().hasLValue()) {
else {
     CGF.EmitAggregateCopy(Dst, SrcLV, Ty, AggValueSlot::DoesNotOverlap,
                           HasLV ? LV.isVolatileQualified()
                                 : RV.isVolatileQualified());
   }
   IsUsed = true;
 }
 
 void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
-                                  QualType type) {
+                                  QualType type,
+                                  const bool CalleeReturnsReference) {
   DisableDebugLocationUpdates Dis(*this, E);
   if (const ObjCIndirectCopyRestoreExpr *CRE
         = dyn_cast<ObjCIndirectCopyRestoreExpr>(E)) {
     assert(getLangOpts().ObjCAutoRefCount);
     return emitWritebackArg(*this, args, CRE);
   }
 
   assert(type->isReferenceType() == E->isGLValue() &&
void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
   if (HasAggregateEvalKind && isa<ImplicitCastExpr>(E) &&
       cast<CastExpr>(E)->getCastKind() == CK_LValueToRValue) {
     LValue L = EmitLValue(cast<CastExpr>(E)->getSubExpr());
     assert(L.isSimple());
     args.addUncopiedAggregate(L, type);
     return;
   }
 
-  args.add(EmitAnyExprToTemp(E), type);
+  AggValueSlot ArgSlot = AggValueSlot::ignored();
+  // If the callee returns a reference, skip this stack saving optimization;
+  // we don't want to prematurely end the lifetime of the temporary. It may be
+  // possible to still perform this optimization if the return type is a
+  // reference to a different type than the parameter.
+  if (!CalleeReturnsReference && hasAggregateEvaluationKind(E->getType())) {
+    Address ArgSlotAlloca = Address::invalid();
+    ArgSlot = CreateAggTemp(E->getType(), "agg.tmp", &ArgSlotAlloca);
+
+    // Emit a lifetime start/end for this temporary. If the type has a
+    // destructor, then we need to keep it alive. FIXME: We should still be able
+    // to end the lifetime after the destructor returns.
+    if (!E->getType().isDestructedType()) {
+      llvm::TypeSize size =
+          CGM.getDataLayout().getTypeAllocSize(ConvertTypeForMem(E->getType()));
+      if (llvm::Value *lifetimeSize =
+              EmitLifetimeStart(size, ArgSlotAlloca.getPointer()))
+        args.addLifetimeCleanup({ArgSlotAlloca.getPointer(), lifetimeSize});
+    }
+  }
+
+  args.add(EmitAnyExpr(E, ArgSlot), type);
 }
 
 QualType CodeGenFunction::getVarArgType(const Expr *Arg) {
   // System headers on Windows define NULL to 0 instead of 0LL on Win64. MSVC
   // implicitly widens null pointer constants that are arguments to varargs
   // functions to pointer-sized ints.
   if (!getTarget().getTriple().isOSWindows())
     return Arg->getType();
if (Ret.isScalar() && TargetDecl) {
     AllocAlignAttrEmitter.EmitAsAnAssumption(Loc, RetTy, Ret);
   }
 
   // Explicitly call CallLifetimeEnd::Emit just to re-use the code even though
   // we can't use the full cleanup mechanism.
   for (CallLifetimeEnd &LifetimeEnd : CallLifetimeEndAfterCall)
     LifetimeEnd.Emit(*this, /*Flags=*/{});
 
+  for (const CallArgList::EndLifetimeInfo &LT : CallArgs.getLifetimeCleanups())
+    EmitLifetimeEnd(LT.Size, LT.Addr);
+
   if (!ReturnValue.isExternallyDestructed() &&
       RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct)
     pushDestroy(QualType::DK_nontrivial_c_struct, Ret.getAggregateAddress(),
                 RetTy);
 
   return Ret;
 }
 

clang/lib/CodeGen/CodeGenFunction.h

public:
   void EmitCfiCheckFail();
 
   /// Create a check for a function parameter that may potentially be
   /// declared as non-null.
   void EmitNonNullArgCheck(RValue RV, QualType ArgType, SourceLocation ArgLoc,
                            AbstractCallee AC, unsigned ParmNum);
 
   /// EmitCallArg - Emit a single call argument.
-  void EmitCallArg(CallArgList &args, const Expr *E, QualType ArgType);
+  void EmitCallArg(CallArgList &args, const Expr *E, QualType ArgType,
+                   const bool CalleeReturnsReference);
 
   /// EmitDelegateCallArg - We are performing a delegate call; that
   /// is, the current function is delegating to another one.  Produce
   /// a r-value suitable for passing the given parameter.
   void EmitDelegateCallArg(CallArgList &args, const VarDecl *param,
                            SourceLocation loc);
 
   /// SetFPAccuracy - Set the minimum required accuracy of the given floating

clang/test/CodeGen/lifetime-call-temp.c

+// RUN: %clang -cc1 -triple x86_64-apple-macos -O1 -disable-llvm-passes %s -S \
+// RUN:   -emit-llvm -o - | FileCheck %s --implicit-check-not=llvm.lifetime
+// RUN: %clang -cc1 -xc++ -std=c++17 -triple x86_64-apple-macos -O1 \
+// RUN:   -disable-llvm-passes %s -S -emit-llvm -o - -Wno-return-type-c-linkage | \
+// RUN:   FileCheck %s --implicit-check-not=llvm.lifetime --check-prefixes=CHECK,CXX
+// RUN: %clang -cc1 -xobjective-c -triple x86_64-apple-macos -O1 \
+// RUN:   -disable-llvm-passes %s -S -emit-llvm -o - | \
+// RUN:   FileCheck %s --implicit-check-not=llvm.lifetime --check-prefixes=CHECK,OBJC
+
+typedef struct { int x[100]; } aggregate;
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+void takes_aggregate(aggregate);
+aggregate gives_aggregate();
+
+// CHECK-LABEL: define void @t1
+void t1() {
+  takes_aggregate(gives_aggregate());
+
+  // CHECK: [[AGGTMP:%.*]] = alloca %struct.aggregate, align 8
+  // CHECK: call void @llvm.lifetime.start.p0(i64 400, ptr [[AGGTMP]])
+  // CHECK: call void{{.*}} @gives_aggregate(ptr sret(%struct.aggregate) align 4 [[AGGTMP]])
+  // CHECK: call void @takes_aggregate(ptr noundef byval(%struct.aggregate) align 8 [[AGGTMP]])
+  // CHECK: call void @llvm.lifetime.end.p0(i64 400, ptr [[AGGTMP]])
+}
+
+// CHECK: declare {{.*}}llvm.lifetime.start
+// CHECK: declare {{.*}}llvm.lifetime.end
+
+#ifdef __cplusplus
+// CXX: define void @t2
+void t2() {
+  struct S {
+    S(aggregate) {}
+  };
+  S{gives_aggregate()};
+
+  // CXX: [[AGG:%.*]] = alloca %struct.aggregate
+  // CXX: call void @llvm.lifetime.start.p0(i64 400, ptr
+  // CXX: call void @gives_aggregate(ptr sret(%struct.aggregate) align 4 [[AGG]])
+  // CXX: call void @_ZZ2t2EN1SC1E9aggregate(ptr {{.*}}, ptr {{.*}} byval(%struct.aggregate) align 8 [[AGG]])
+  // CXX: call void @llvm.lifetime.end.p0(i64 400, ptr
+}
+
+struct Dtor {
+  ~Dtor();
+};
+
+void takes_dtor(Dtor);
+Dtor gives_dtor();
+
+// CXX: define void @t3
+void t3() {
+  takes_dtor(gives_dtor());
+
+  // CXX-NOT: @llvm.lifetime
+  // CXX: ret void
+}
+
+struct foo { int x; };
+foo &min(foo a, foo b);
+
+void consume (foo&);
+
+// The intent of this test is that we skip the lifetime markers for the temp
+// aggregates because the callee returns a reference. Maybe it's possible to
+// improve this in the future?
+void bar () {
+  foo a, b;
+  // CXX: call void @llvm.lifetime.start.p0(i64 4, ptr %a)
+  // CXX: call void @llvm.lifetime.start.p0(i64 4, ptr %b)
+  // CXX: call void @llvm.lifetime.end.p0(i64 4, ptr %b)
+  // CXX: call void @llvm.lifetime.end.p0(i64 4, ptr %a)
+  // CXX-NOT: call void @llvm.lifetime.start.p0(i64 4, ptr %agg.tmp)
+  // CXX-NOT: call void @llvm.lifetime.start.p0(i64 4, ptr %agg.tmp1)
+  // CXX-NOT: call void @llvm.lifetime.end.p0(i64 4, ptr %agg.tmp)
+  // CXX-NOT: call void @llvm.lifetime.end.p0(i64 4, ptr %agg.tmp1)
+  consume(min(a, b));
+}
+
+
+#endif
+
+#ifdef __OBJC__
+
+@interface X
+-m:(aggregate)x;
+@end
+
+// OBJC: define void @t4
+void t4(X *x) {
+  [x m: gives_aggregate()];
+
+  // OBJC: [[AGG:%.*]] = alloca %struct.aggregate
+  // OBJC: call void @llvm.lifetime.start.p0(i64 400, ptr
+  // OBJC: call void{{.*}} @gives_aggregate(ptr sret(%struct.aggregate) align 4 [[AGGTMP]])
+  // OBJC: call {{.*}}@objc_msgSend
+  // OBJC: call void @llvm.lifetime.end.p0(i64 400, ptr
+}
+
+#endif
+
+#ifdef __cplusplus
+}
+#endif

clang/test/CodeGenCXX/amdgcn-call-with-aggarg.cpp

+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -emit-llvm -O3 -disable-llvm-passes -o - %s | FileCheck %s
+
+struct A {
+  float x, y, z, w;
+};
+
+void foo(A a);
+
+// CHECK-LABEL: @_Z4testv
+// CHECK:         [[A:%.*]] = alloca [[STRUCT_A:%.*]], align 4, addrspace(5)
+// CHECK-NEXT:    [[AGG_TMP:%.*]] = alloca [[STRUCT_A]], align 4, addrspace(5)
+// CHECK-NEXT:    [[A_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[A]] to ptr
+// CHECK-NEXT:    [[AGG_TMP_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[AGG_TMP]] to ptr
+// CHECK-NEXT:    call void @llvm.lifetime.start.p5(i64 16, ptr addrspace(5) [[A]]) #[[ATTR4:[0-9]+]]
+// CHECK-NEXT:    call void @llvm.lifetime.start.p5(i64 16, ptr addrspace(5) [[AGG_TMP]]) #[[ATTR4]]
+void test() {
+  A a;
+  foo(a);
+}

clang/test/CodeGenCXX/stack-reuse-miscompile.cpp

 {
 // It's essential that the lifetimes of all three T temporaries here are
 // overlapping. They must all remain alive through the call to str().
 //
 // CHECK: [[T1:%.*]] = alloca %class.T, align 4
 // CHECK: [[T2:%.*]] = alloca %class.T, align 4
 // CHECK: [[T3:%.*]] = alloca %class.T, align 4
 //
+// CHECK: [[AGG:%.*]] = alloca %class.S, align 4
+//
 // FIXME: We could defer starting the lifetime of the return object of concat
 // until the call.
 // CHECK: call void @llvm.lifetime.start.p0(i64 16, ptr [[T1]])
 //
 // CHECK: call void @llvm.lifetime.start.p0(i64 16, ptr [[T2]])
 // CHECK: [[T4:%.*]] = call noundef ptr @_ZN1TC1EPKc(ptr {{[^,]*}} [[T2]], ptr noundef @.str)
 //
 // CHECK: call void @llvm.lifetime.start.p0(i64 16, ptr [[T3]])
+// CHECK: call void @llvm.lifetime.start.p0(i64 8, ptr [[AGG]])
 // CHECK: [[T5:%.*]] = call noundef ptr @_ZN1TC1E1S(ptr {{[^,]*}} [[T3]], [2 x i32] %{{.*}})
+// CHECK: call void @llvm.lifetime.end.p0(i64 8, ptr [[AGG]]
 //
 // CHECK: call void @_ZNK1T6concatERKS_(ptr sret(%class.T) align 4 [[T1]], ptr {{[^,]*}} [[T2]], ptr noundef nonnull align 4 dereferenceable(16) [[T3]])
 // CHECK: [[T6:%.*]] = call noundef ptr @_ZNK1T3strEv(ptr {{[^,]*}} [[T1]])
 //
 // CHECK: call void @llvm.lifetime.end.p0(
 // CHECK: call void @llvm.lifetime.end.p0(
 // CHECK: call void @llvm.lifetime.end.p0(
 // CHECK: ret ptr [[T6]]
 
   return T("[").concat(T(s)).str();
 }
 
 // CHECK: declare {{.*}}llvm.lifetime.start
 // CHECK: declare {{.*}}llvm.lifetime.end

clang/test/CodeGenCoroutines/pr59181.cpp

Task bar(bool cond) {
   co_return cond ? Task{ co_await Task{}}: Task{};
 }
 
 void foo() {
   bar(true);
 }
 
 // CHECK: cleanup.cont:{{.*}}
+// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 4, ptr [[AGG:%agg.tmp[0-9]+]])
 // CHECK-NEXT: load i8
 // CHECK-NEXT: trunc
 // CHECK-NEXT: store i1 false
 // CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 8, ptr [[REF:%ref.tmp[0-9]+]])
 
 // CHECK: await.suspend:{{.*}}
 // CHECK-NOT: call void @llvm.lifetime.start.p0(i64 8, ptr [[REF]])
+// CHECK-NOT: call void @llvm.lifetime.start.p0(i64 8, ptr [[AGG]])
 // CHECK: call void @_ZZN4Task12promise_type15await_transformES_EN10Suspension13await_suspendESt16coroutine_handleIvE
+// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 8, ptr [[AGG2:%agg.tmp[0-9]+]])
 // CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 8, ptr [[REF]])