llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-coroutines Author: Yuxuan Chen (yuxuanchen1997) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/94693.diff 10 Files Affected: - (modified) clang/include/clang/Basic/Attr.td (+8) - (modified) clang/include/clang/Basic/AttrDocs.td (+20) - (modified) clang/lib/CodeGen/CGCoroutine.cpp (+71-5) - (modified) clang/lib/CodeGen/CGExpr.cpp (+5-1) - (modified) clang/lib/CodeGen/CodeGenFunction.h (+3) - (modified) clang/test/Misc/pragma-attribute-supported-attributes-list.test (+1) - (modified) llvm/include/llvm/IR/Intrinsics.td (+3) - (modified) llvm/lib/Transforms/Coroutines/CoroCleanup.cpp (+8-3) - (modified) llvm/lib/Transforms/Coroutines/CoroElide.cpp (+53-3) - (modified) llvm/lib/Transforms/Coroutines/Coroutines.cpp (+1) ``````````diff diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 17d9a710d948b..0fb623d6c0515 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -1212,6 +1212,14 @@ def CoroDisableLifetimeBound : InheritableAttr { let SimpleHandler = 1; } +def CoroStructuredConcurrencyType : InheritableAttr { + let Spellings = [Clang<"coro_structured_concurrency">]; + let Subjects = SubjectList<[CXXRecord]>; + let LangOpts = [CPlusPlus]; + let Documentation = [CoroStructuredConcurrencyDoc]; + let SimpleHandler = 1; +} + // OSObject-based attributes. def OSConsumed : InheritableParamAttr { let Spellings = [Clang<"os_consumed">]; diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td index 70d5dfa8aaf86..50fc0fc2d16f6 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -8015,6 +8015,26 @@ but do not pass them to the underlying coroutine or pass them by value. }]; } +def CoroStructuredConcurrencyDoc : Documentation { + let Category = DocCatDecl; + let Content = [{ +The ``[[clang::coro_structured_concurrency]]`` is a class attribute which can be applied +to a coroutine return type. + +When a coroutine function that returns such a type calls another coroutine function, +the compiler performs heap allocation elision when the following conditions are all met: +- callee coroutine function returns a type that is annotated with + ``[[clang::coro_structured_concurrency]]``. +- The callee coroutine function is inlined. +- In caller coroutine, the return value of the callee is a prvalue or an xvalue, and +- The temporary expression containing the callee coroutine object is immediately co_awaited. + +The behavior is undefined if any of the following condition was met: +- the caller coroutine is destroyed earlier than the callee coroutine. + + }]; +} + def CountedByDocs : Documentation { let Category = DocCatField; let Content = [{ diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index b4c724422c14a..78ae04982cba0 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -12,9 +12,12 @@ #include "CGCleanup.h" #include "CodeGenFunction.h" -#include "llvm/ADT/ScopeExit.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtVisitor.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/ScopeExit.h" +#include "llvm/IR/Intrinsics.h" using namespace clang; using namespace CodeGen; @@ -219,17 +222,81 @@ namespace { RValue RV; }; } + +static MaterializeTemporaryExpr * +getStructuredConcurrencyOperand(ASTContext &Ctx, + CoroutineSuspendExpr const &S) { + auto *E = S.getCommonExpr(); + auto *Temporary = dyn_cast_or_null<MaterializeTemporaryExpr>(E); + if (!Temporary) + return nullptr; + + auto *Operator = + dyn_cast_or_null<CXXOperatorCallExpr>(Temporary->getSubExpr()); + + if (!Operator || + Operator->getOperator() != OverloadedOperatorKind::OO_Coawait || + Operator->getNumArgs() != 1) + return nullptr; + + Expr *Arg = Operator->getArg(0); + assert(Arg && "Arg to operator co_await should not be null"); + auto *CalleeRetClass = Arg->getType()->getAsCXXRecordDecl(); + + if (!CalleeRetClass || + !CalleeRetClass->hasAttr<CoroStructuredConcurrencyTypeAttr>()) + return nullptr; + + if (!Arg->isTemporaryObject(Ctx, CalleeRetClass)) { + return nullptr; + } + + return dyn_cast<MaterializeTemporaryExpr>(Arg); +} + static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro, CoroutineSuspendExpr const &S, AwaitKind Kind, AggValueSlot aggSlot, bool ignoreResult, bool forLValue) { auto *E = S.getCommonExpr(); + auto &Builder = CGF.Builder; + bool MarkOperandSafeIntrinsic = false; + + auto *TemporaryOperand = [&]() -> MaterializeTemporaryExpr * { + bool CurFnRetTyHasAttr = false; + if (auto *RetTyPtr = CGF.FnRetTy.getTypePtrOrNull()) { + if (auto *CxxRecord = RetTyPtr->getAsCXXRecordDecl()) { + CurFnRetTyHasAttr = + CxxRecord->hasAttr<CoroStructuredConcurrencyTypeAttr>(); + } + } + + if (CurFnRetTyHasAttr) { + return getStructuredConcurrencyOperand(CGF.getContext(), S); + } + return nullptr; + }(); + + if (TemporaryOperand) { + CGF.TemporaryValues[TemporaryOperand] = nullptr; + MarkOperandSafeIntrinsic = true; + } + auto CommonBinder = CodeGenFunction::OpaqueValueMappingData::bind(CGF, S.getOpaqueValue(), E); auto UnbindCommonOnExit = llvm::make_scope_exit([&] { CommonBinder.unbind(CGF); }); + if (MarkOperandSafeIntrinsic) { + auto It = CGF.TemporaryValues.find(TemporaryOperand); + assert(It != CGF.TemporaryValues.end()); + if (auto Value = It->second) + Builder.CreateCall(CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_safe_elide), + {Value}); + CGF.TemporaryValues.erase(TemporaryOperand); + } + auto Prefix = buildSuspendPrefixStr(Coro, Kind); BasicBlock *ReadyBlock = CGF.createBasicBlock(Prefix + Twine(".ready")); BasicBlock *SuspendBlock = CGF.createBasicBlock(Prefix + Twine(".suspend")); @@ -241,7 +308,6 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co // Otherwise, emit suspend logic. CGF.EmitBlock(SuspendBlock); - auto &Builder = CGF.Builder; llvm::Function *CoroSave = CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_save); auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy); auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr}); @@ -255,9 +321,9 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co "expected to be called in coroutine context"); SmallVector<llvm::Value *, 3> SuspendIntrinsicCallArgs; - SuspendIntrinsicCallArgs.push_back( - CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF)); - + auto *BoundAwaiterValue = + CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF); + SuspendIntrinsicCallArgs.push_back(BoundAwaiterValue); SuspendIntrinsicCallArgs.push_back(CGF.CurCoro.Data->CoroBegin); SuspendIntrinsicCallArgs.push_back(SuspendWrapper); diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index d6478cc6835d8..139a602650b6d 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -618,7 +618,11 @@ EmitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *M) { } } - return MakeAddrLValue(Object, M->getType(), AlignmentSource::Decl); + auto Ret = MakeAddrLValue(Object, M->getType(), AlignmentSource::Decl); + if (TemporaryValues.contains(M)) { + TemporaryValues[M] = Ret.getPointer(*this); + } + return Ret; } RValue diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 5739fbaaa9194..083cb8e2f376e 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -371,6 +371,9 @@ class CodeGenFunction : public CodeGenTypeCache { }; CGCoroInfo CurCoro; + llvm::SmallDenseMap<const MaterializeTemporaryExpr *, llvm::Value *> + TemporaryValues; + bool isCoroutine() const { return CurCoro.Data != nullptr; } diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test index 99732694f72a5..0369906d7c275 100644 --- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test +++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test @@ -62,6 +62,7 @@ // CHECK-NEXT: CoroLifetimeBound (SubjectMatchRule_record) // CHECK-NEXT: CoroOnlyDestroyWhenComplete (SubjectMatchRule_record) // CHECK-NEXT: CoroReturnType (SubjectMatchRule_record) +// CHECK-NEXT: CoroStructuredConcurrencyType (SubjectMatchRule_record) // CHECK-NEXT: CoroWrapper (SubjectMatchRule_function) // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td index 107442623ab7b..8e554d99ee3b6 100644 --- a/llvm/include/llvm/IR/Intrinsics.td +++ b/llvm/include/llvm/IR/Intrinsics.td @@ -1729,6 +1729,9 @@ def int_coro_subfn_addr : DefaultAttrsIntrinsic< [IntrReadMem, IntrArgMemOnly, ReadOnly<ArgIndex<0>>, NoCapture<ArgIndex<0>>]>; +def int_coro_safe_elide : DefaultAttrsIntrinsic< + [], [llvm_ptr_ty], []>; + ///===-------------------------- Other Intrinsics --------------------------===// // // TODO: We should introduce a new memory kind fo traps (and other side effects diff --git a/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp b/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp index 3e3825fcd50e2..71229eae5cb47 100644 --- a/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp @@ -8,10 +8,11 @@ #include "llvm/Transforms/Coroutines/CoroCleanup.h" #include "CoroInternal.h" +#include "llvm/IR/Function.h" #include "llvm/IR/IRBuilder.h" #include "llvm/IR/InstIterator.h" +#include "llvm/IR/Intrinsics.h" #include "llvm/IR/PassManager.h" -#include "llvm/IR/Function.h" #include "llvm/Transforms/Scalar/SimplifyCFG.h" using namespace llvm; @@ -80,7 +81,7 @@ bool Lowerer::lower(Function &F) { } else continue; break; - case Intrinsic::coro_async_size_replace: + case Intrinsic::coro_async_size_replace: { auto *Target = cast<ConstantStruct>( cast<GlobalVariable>(II->getArgOperand(0)->stripPointerCasts()) ->getInitializer()); @@ -98,6 +99,9 @@ bool Lowerer::lower(Function &F) { Target->replaceAllUsesWith(NewFuncPtrStruct); break; } + case Intrinsic::coro_safe_elide: + break; + } II->eraseFromParent(); Changed = true; } @@ -111,7 +115,8 @@ static bool declaresCoroCleanupIntrinsics(const Module &M) { M, {"llvm.coro.alloc", "llvm.coro.begin", "llvm.coro.subfn.addr", "llvm.coro.free", "llvm.coro.id", "llvm.coro.id.retcon", "llvm.coro.id.async", "llvm.coro.id.retcon.once", - "llvm.coro.async.size.replace", "llvm.coro.async.resume"}); + "llvm.coro.async.size.replace", "llvm.coro.async.resume", + "llvm.coro.safe.elide"}); } PreservedAnalyses CoroCleanupPass::run(Module &M, diff --git a/llvm/lib/Transforms/Coroutines/CoroElide.cpp b/llvm/lib/Transforms/Coroutines/CoroElide.cpp index 74b5ccb7b9b71..dd2f72410c931 100644 --- a/llvm/lib/Transforms/Coroutines/CoroElide.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroElide.cpp @@ -7,12 +7,14 @@ //===----------------------------------------------------------------------===// #include "llvm/Transforms/Coroutines/CoroElide.h" +#include "CoroInstr.h" #include "CoroInternal.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/Statistic.h" #include "llvm/Analysis/AliasAnalysis.h" #include "llvm/Analysis/InstructionSimplify.h" #include "llvm/Analysis/OptimizationRemarkEmitter.h" +#include "llvm/Analysis/PostDominators.h" #include "llvm/IR/Dominators.h" #include "llvm/IR/InstIterator.h" #include "llvm/Support/ErrorHandling.h" @@ -56,7 +58,8 @@ class FunctionElideInfo { class CoroIdElider { public: CoroIdElider(CoroIdInst *CoroId, FunctionElideInfo &FEI, AAResults &AA, - DominatorTree &DT, OptimizationRemarkEmitter &ORE); + DominatorTree &DT, PostDominatorTree &PDT, + OptimizationRemarkEmitter &ORE); void elideHeapAllocations(uint64_t FrameSize, Align FrameAlign); bool lifetimeEligibleForElide() const; bool attemptElide(); @@ -68,6 +71,7 @@ class CoroIdElider { FunctionElideInfo &FEI; AAResults &AA; DominatorTree &DT; + PostDominatorTree &PDT; OptimizationRemarkEmitter &ORE; SmallVector<CoroBeginInst *, 1> CoroBegins; @@ -183,8 +187,9 @@ void FunctionElideInfo::collectPostSplitCoroIds() { CoroIdElider::CoroIdElider(CoroIdInst *CoroId, FunctionElideInfo &FEI, AAResults &AA, DominatorTree &DT, + PostDominatorTree &PDT, OptimizationRemarkEmitter &ORE) - : CoroId(CoroId), FEI(FEI), AA(AA), DT(DT), ORE(ORE) { + : CoroId(CoroId), FEI(FEI), AA(AA), DT(DT), PDT(PDT), ORE(ORE) { // Collect all coro.begin and coro.allocs associated with this coro.id. for (User *U : CoroId->users()) { if (auto *CB = dyn_cast<CoroBeginInst>(U)) @@ -336,6 +341,41 @@ bool CoroIdElider::canCoroBeginEscape( return false; } +// FIXME: This is not accounting for the stores to tasks whose handle is not +// zero offset. +static const StoreInst *getPostDominatingStoreToTask(const CoroBeginInst *CB, + PostDominatorTree &PDT) { + const StoreInst *OnlyStore = nullptr; + + for (auto *U : CB->users()) { + auto *Store = dyn_cast<StoreInst>(U); + if (Store && Store->getValueOperand() == CB) { + if (OnlyStore) { + // Store must be unique. one coro begin getting stored to multiple + // stores is not accepted. + return nullptr; + } + OnlyStore = Store; + } + } + + if (!OnlyStore || !PDT.dominates(OnlyStore, CB)) { + return nullptr; + } + + return OnlyStore; +} + +static bool isMarkedSafeElide(const llvm::Value *V) { + for (auto *U : V->users()) { + auto *II = dyn_cast<IntrinsicInst>(U); + if (II && (II->getIntrinsicID() == Intrinsic::coro_safe_elide)) { + return true; + } + } + return false; +} + bool CoroIdElider::lifetimeEligibleForElide() const { // If no CoroAllocs, we cannot suppress allocation, so elision is not // possible. @@ -364,6 +404,15 @@ bool CoroIdElider::lifetimeEligibleForElide() const { // Filter out the coro.destroy that lie along exceptional paths. for (const auto *CB : CoroBegins) { + // This might be too strong of a condition but should be very safe. + // If the CB is unconditionally stored into a "Task Like Object", + // and such object is "safe elide". + if (auto *MaybeStoreToTask = getPostDominatingStoreToTask(CB, PDT)) { + auto Dest = MaybeStoreToTask->getPointerOperand(); + if (isMarkedSafeElide(Dest)) + continue; + } + auto It = DestroyAddr.find(CB); // FIXME: If we have not found any destroys for this coro.begin, we @@ -476,11 +525,12 @@ PreservedAnalyses CoroElidePass::run(Function &F, FunctionAnalysisManager &AM) { AAResults &AA = AM.getResult<AAManager>(F); DominatorTree &DT = AM.getResult<DominatorTreeAnalysis>(F); + PostDominatorTree &PDT = AM.getResult<PostDominatorTreeAnalysis>(F); auto &ORE = AM.getResult<OptimizationRemarkEmitterAnalysis>(F); bool Changed = false; for (auto *CII : FEI.getCoroIds()) { - CoroIdElider CIE(CII, FEI, AA, DT, ORE); + CoroIdElider CIE(CII, FEI, AA, DT, PDT, ORE); Changed |= CIE.attemptElide(); } diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index 1a92bc1636257..48c02e5406b75 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -86,6 +86,7 @@ static const char *const CoroIntrinsics[] = { "llvm.coro.prepare.retcon", "llvm.coro.promise", "llvm.coro.resume", + "llvm.coro.safe.elide", "llvm.coro.save", "llvm.coro.size", "llvm.coro.subfn.addr", `````````` </details> https://github.com/llvm/llvm-project/pull/94693 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
