llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Hans Wennborg (zmodem) <details> <summary>Changes</summary> Metadata should not be "load bearing", i.e. required for correctness, since optimizations are not required to preserve it. Therefore, turn this into an intrinsic instead. This is a follow-up to #<!-- -->127653. --- Full diff: https://github.com/llvm/llvm-project/pull/129255.diff 15 Files Affected: - (modified) clang/lib/CodeGen/CGCoroutine.cpp (+5-6) - (modified) clang/test/CodeGenCoroutines/coro-gro.cpp (+2-2) - (modified) clang/test/CodeGenCoroutines/coro-params.cpp (+4-5) - (modified) llvm/docs/Coroutines.rst (+31-19) - (modified) llvm/docs/ReleaseNotes.md (+4) - (modified) llvm/include/llvm/IR/FixedMetadataKinds.def (+2-1) - (modified) llvm/include/llvm/IR/Intrinsics.td (+1) - (modified) llvm/include/llvm/Transforms/Coroutines/CoroInstr.h (+16) - (modified) llvm/include/llvm/Transforms/Coroutines/CoroShape.h (+2) - (modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (+6) - (modified) llvm/lib/Transforms/Coroutines/Coroutines.cpp (+3) - (modified) llvm/lib/Transforms/Coroutines/SpillUtils.cpp (+9-5) - (modified) llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp (+2-1) - (modified) llvm/test/Transforms/Coroutines/coro-alloca-outside-frame.ll (+21-9) - (added) llvm/test/Transforms/Mem2Reg/ignore-coro-outside-frame.ll (+13) ``````````diff diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index a9795c2c0dc8f..7bd3208990572 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -709,8 +709,8 @@ struct GetReturnObjectManager { auto *GroAlloca = dyn_cast_or_null<llvm::AllocaInst>( GroEmission.getOriginalAllocatedAddress().getPointer()); assert(GroAlloca && "expected alloca to be emitted"); - GroAlloca->setMetadata(llvm::LLVMContext::MD_coro_outside_frame, - llvm::MDNode::get(CGF.CGM.getLLVMContext(), {})); + Builder.CreateCall( + CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_outside_frame), {GroAlloca}); // Remember the top of EHStack before emitting the cleanup. auto old_top = CGF.EHStack.stable_begin(); @@ -863,10 +863,9 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { // in the MSVC C++ ABI, are appropriately destroyed after setting up the // coroutine. Address ParmAddr = GetAddrOfLocalVar(Parm); - if (auto *ParmAlloca = - dyn_cast<llvm::AllocaInst>(ParmAddr.getBasePointer())) { - ParmAlloca->setMetadata(llvm::LLVMContext::MD_coro_outside_frame, - llvm::MDNode::get(CGM.getLLVMContext(), {})); + if (auto *PA = dyn_cast<llvm::AllocaInst>(ParmAddr.getBasePointer())) { + Builder.CreateCall( + CGM.getIntrinsic(llvm::Intrinsic::coro_outside_frame), {PA}); } } for (auto *PM : S.getParamMoves()) { diff --git a/clang/test/CodeGenCoroutines/coro-gro.cpp b/clang/test/CodeGenCoroutines/coro-gro.cpp index b62134317cef2..bfcfc641dcd72 100644 --- a/clang/test/CodeGenCoroutines/coro-gro.cpp +++ b/clang/test/CodeGenCoroutines/coro-gro.cpp @@ -30,11 +30,12 @@ void doSomething() noexcept; int f() { // CHECK: %[[RetVal:.+]] = alloca i32 // CHECK: %[[GroActive:.+]] = alloca i1 - // CHECK: %[[CoroGro:.+]] = alloca %struct.GroType, {{.*}} !coro.outside.frame ![[OutFrameMetadata:.+]] + // CHECK: %[[CoroGro:.+]] = alloca %struct.GroType // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64() // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]]) // CHECK: store i1 false, ptr %[[GroActive]] + // CHECK: call void @llvm.coro.outside.frame(ptr %[[CoroGro]]) // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeC1Ev( // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv({{.*}} %[[CoroGro]] // CHECK: store i1 true, ptr %[[GroActive]] @@ -106,4 +107,3 @@ invoker g() { // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]] co_return; } -// CHECK: ![[OutFrameMetadata]] = !{} \ No newline at end of file diff --git a/clang/test/CodeGenCoroutines/coro-params.cpp b/clang/test/CodeGenCoroutines/coro-params.cpp index 719726cca29c5..792500ce5ac23 100644 --- a/clang/test/CodeGenCoroutines/coro-params.cpp +++ b/clang/test/CodeGenCoroutines/coro-params.cpp @@ -72,13 +72,13 @@ void consume(int,int,int,int) noexcept; // CHECK: define{{.*}} void @_Z1fi8MoveOnly11MoveAndCopy10TrivialABI(i32 noundef %val, ptr noundef %[[MoParam:.+]], ptr noundef %[[McParam:.+]], i32 %[[TrivialParam:.+]]) #0 personality ptr @__gxx_personality_v0 void f(int val, MoveOnly moParam, MoveAndCopy mcParam, TrivialABI trivialParam) { // CHECK: %[[TrivialAlloca:.+]] = alloca %struct.TrivialABI, - // CHECK-SAME: !coro.outside.frame // CHECK: %[[MoCopy:.+]] = alloca %struct.MoveOnly, // CHECK: %[[McCopy:.+]] = alloca %struct.MoveAndCopy, // CHECK: %[[TrivialCopy:.+]] = alloca %struct.TrivialABI, // CHECK: store i32 %val, ptr %[[ValAddr:.+]] // CHECK: call ptr @llvm.coro.begin( + // CHECK: @llvm.coro.outside.frame(ptr %[[TrivialAlloca]]) // CHECK: call void @_ZN8MoveOnlyC1EOS_(ptr {{[^,]*}} %[[MoCopy]], ptr noundef nonnull align 4 dereferenceable(4) %[[MoParam]]) // CHECK-NEXT: call void @llvm.lifetime.start.p0( // CHECK-NEXT: call void @_ZN11MoveAndCopyC1EOS_(ptr {{[^,]*}} %[[McCopy]], ptr noundef nonnull align 4 dereferenceable(4) %[[McParam]]) # @@ -226,13 +226,12 @@ void consume(int) noexcept; // may be destroyed before the destructor call. void msabi(MSParm p) { // MSABI: define{{.*}} void @"?msabi@@YAXUMSParm@@@Z"(i32 %[[Param:.+]]) - - // The parameter's local alloca is marked not part of the frame. // MSABI: %[[ParamAlloca:.+]] = alloca %struct.MSParm - // MSABI-SAME: !coro.outside.frame - // MSABI: %[[ParamCopy:.+]] = alloca %struct.MSParm + // The parameter's local alloca is marked not part of the frame. + // MSABI: call void @llvm.coro.outside.frame(ptr %[[ParamAlloca]]) + consume(p.val); // The parameter's copy is used by the coroutine. // MSABI: %[[ValPtr:.+]] = getelementptr inbounds nuw %struct.MSParm, ptr %[[ParamCopy]], i32 0, i32 0 diff --git a/llvm/docs/Coroutines.rst b/llvm/docs/Coroutines.rst index 60e32dc467d27..55bc72026c1ed 100644 --- a/llvm/docs/Coroutines.rst +++ b/llvm/docs/Coroutines.rst @@ -1641,6 +1641,37 @@ the function call. ptr %ctxt, ptr %task, ptr %actor) unreachable + +.. _coro.outside.frame: + +'llvm.coro.outside.frame' Intrinsic +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +:: + + declare void @llvm.coro.outside.frame(ptr %p) + +Overview: +""""""""" + +Signifies that an alloca shouldn't be promoted to the coroutine frame. + +Arguments: +"""""""""" + +An alloca that should remain outside the coroutine frame. + +Semantics: +"""""""""" + +Calling `@llvm.coro.outside.frame` with an alloca signifies that the alloca +should not be promoted to the coroutine frame. This allows the frontend to +ensure that e.g. callee-destructed parameters are allocated on the stack of the +ramp function, and thus available until the function returns. + +The intrinsic should only be used in presplit coroutines, and is consumed by +the CoroSplit pass. + + .. _coro.suspend: .. _suspend points: @@ -2179,25 +2210,6 @@ or deallocations that may be guarded by `@llvm.coro.alloc` and `@llvm.coro.free` CoroAnnotationElidePass performs the heap elision when possible. Note that for recursive or mutually recursive functions this elision is usually not possible. -Metadata -======== - -'``coro.outside.frame``' Metadata ---------------------------------- - -``coro.outside.frame`` metadata may be attached to an alloca instruction to -to signify that it shouldn't be promoted to the coroutine frame, useful for -filtering allocas out by the frontend when emitting internal control mechanisms. -Additionally, this metadata is only used as a flag, so the associated -node must be empty. - -.. code-block:: text - - %__coro_gro = alloca %struct.GroType, align 1, !coro.outside.frame !0 - - ... - !0 = !{} - Areas Requiring Attention ========================= #. When coro.suspend returns -1, the coroutine is suspended, and it's possible diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md index 12dd09ad41135..24e517f9942f5 100644 --- a/llvm/docs/ReleaseNotes.md +++ b/llvm/docs/ReleaseNotes.md @@ -145,6 +145,10 @@ Changes to the CodeGen infrastructure Changes to the Metadata Info --------------------------------- +* The `coro.outside.frame` metadata has been replaced by [an intrinsic with the + same name](coro.outside.frame). The old metadata is still parsed but has no + effect. + Changes to the Debug Info --------------------------------- diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def index df572e8791e13..712c139c2d57b 100644 --- a/llvm/include/llvm/IR/FixedMetadataKinds.def +++ b/llvm/include/llvm/IR/FixedMetadataKinds.def @@ -50,6 +50,7 @@ LLVM_FIXED_MD_KIND(MD_callsite, "callsite", 35) LLVM_FIXED_MD_KIND(MD_kcfi_type, "kcfi_type", 36) LLVM_FIXED_MD_KIND(MD_pcsections, "pcsections", 37) LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38) -LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39) +LLVM_FIXED_MD_KIND(MD_coro_outside_frame_DEPRECATED, + "coro.outside.frame", 39) // Preserved for compatibility. LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40) LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41) diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td index 62239ca705b9e..2d90bae3a0b64 100644 --- a/llvm/include/llvm/IR/Intrinsics.td +++ b/llvm/include/llvm/IR/Intrinsics.td @@ -1779,6 +1779,7 @@ def int_coro_alloca_alloc : Intrinsic<[llvm_token_ty], [llvm_anyint_ty, llvm_i32_ty], []>; def int_coro_alloca_get : Intrinsic<[llvm_ptr_ty], [llvm_token_ty], []>; def int_coro_alloca_free : Intrinsic<[], [llvm_token_ty], []>; +def int_coro_outside_frame : Intrinsic<[], [llvm_ptr_ty], [IntrNoMem]>; // Coroutine Manipulation Intrinsics. diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroInstr.h b/llvm/include/llvm/Transforms/Coroutines/CoroInstr.h index fbc76219ead86..f24d7214967a3 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroInstr.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroInstr.h @@ -796,6 +796,22 @@ class CoroAllocaFreeInst : public IntrinsicInst { } }; +/// This represents the llvm.coro.outside.frame instruction. +class CoroOutsideFrameInst : public IntrinsicInst { + enum { PtrArg }; + +public: + Value *getPtr() const { return getArgOperand(PtrArg); } + + // Methods to support type inquiry through isa, cast, and dyn_cast: + static bool classof(const IntrinsicInst *I) { + return I->getIntrinsicID() == Intrinsic::coro_outside_frame; + } + static bool classof(const Value *V) { + return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V)); + } +}; + } // End namespace llvm. #endif // LLVM_TRANSFORMS_COROUTINES_COROINSTR_H diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h index ea93ced1ce29e..657c2c8bfdc82 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h @@ -57,6 +57,7 @@ struct Shape { SmallVector<AnyCoroSuspendInst *, 4> CoroSuspends; SmallVector<CoroAwaitSuspendInst *, 4> CoroAwaitSuspends; SmallVector<CallInst *, 2> SymmetricTransfers; + SmallVector<CoroOutsideFrameInst *, 8> OutsideFrames; // Values invalidated by replaceSwiftErrorOps() SmallVector<CallInst *, 2> SwiftErrorOps; @@ -69,6 +70,7 @@ struct Shape { CoroSuspends.clear(); CoroAwaitSuspends.clear(); SymmetricTransfers.clear(); + OutsideFrames.clear(); SwiftErrorOps.clear(); diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp index e1f767edd6ee1..0a9961d3e06b8 100644 --- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp @@ -2015,7 +2015,13 @@ static void doSplitCoroutine(Function &F, SmallVectorImpl<Function *> &Clones, simplifySuspendPoints(Shape); normalizeCoroutine(F, Shape, TTI); + ABI.buildCoroutineFrame(OptimizeFrame); + + // @llvm.coro.outside.frame is not needed after the frame has been built. + for (Instruction *I : Shape.OutsideFrames) + I->eraseFromParent(); + replaceFrameSizeAndAlignment(Shape); bool isNoSuspendCoroutine = Shape.CoroSuspends.empty(); diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index 7b59c39283ded..392f74cd2f52f 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -286,6 +286,9 @@ void coro::Shape::analyze(Function &F, } } break; + case Intrinsic::coro_outside_frame: + OutsideFrames.push_back(cast<CoroOutsideFrameInst>(II)); + break; } } } diff --git a/llvm/lib/Transforms/Coroutines/SpillUtils.cpp b/llvm/lib/Transforms/Coroutines/SpillUtils.cpp index 5062ee97a665d..bfe22699f655c 100644 --- a/llvm/lib/Transforms/Coroutines/SpillUtils.cpp +++ b/llvm/lib/Transforms/Coroutines/SpillUtils.cpp @@ -417,7 +417,8 @@ struct AllocaUseVisitor : PtrUseVisitor<AllocaUseVisitor> { static void collectFrameAlloca(AllocaInst *AI, const coro::Shape &Shape, const SuspendCrossingInfo &Checker, SmallVectorImpl<AllocaInfo> &Allocas, - const DominatorTree &DT) { + const DominatorTree &DT, + const SmallPtrSetImpl<Value*> &OutsideFrameSet) { if (Shape.CoroSuspends.empty()) return; @@ -426,9 +427,8 @@ static void collectFrameAlloca(AllocaInst *AI, const coro::Shape &Shape, if (AI == Shape.SwitchLowering.PromiseAlloca) return; - // The __coro_gro alloca should outlive the promise, make sure we - // keep it outside the frame. - if (AI->hasMetadata(LLVMContext::MD_coro_outside_frame)) + // The alloca has been marked as belonging outside the frame. + if (OutsideFrameSet.contains(AI)) return; // The code that uses lifetime.start intrinsic does not work for functions @@ -464,6 +464,10 @@ void collectSpillsAndAllocasFromInsts( const SuspendCrossingInfo &Checker, const DominatorTree &DT, const coro::Shape &Shape) { + SmallPtrSet<Value*, 4> OutsideFramePtrs; + for (const CoroOutsideFrameInst *I : Shape.OutsideFrames) + OutsideFramePtrs.insert(I->getPtr()); + for (Instruction &I : instructions(F)) { // Values returned from coroutine structure intrinsics should not be part // of the Coroutine Frame. @@ -497,7 +501,7 @@ void collectSpillsAndAllocasFromInsts( continue; if (auto *AI = dyn_cast<AllocaInst>(&I)) { - collectFrameAlloca(AI, Shape, Checker, Allocas, DT); + collectFrameAlloca(AI, Shape, Checker, Allocas, DT, OutsideFramePtrs); continue; } diff --git a/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp b/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp index 05fd989271c32..d6a38a8670c07 100644 --- a/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp +++ b/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp @@ -81,7 +81,8 @@ bool llvm::isAllocaPromotable(const AllocaInst *AI) { return false; } else if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) { if (!II->isLifetimeStartOrEnd() && !II->isDroppable() && - II->getIntrinsicID() != Intrinsic::fake_use) + II->getIntrinsicID() != Intrinsic::fake_use && + II->getIntrinsicID() != Intrinsic::coro_outside_frame) return false; } else if (const BitCastInst *BCI = dyn_cast<BitCastInst>(U)) { if (!onlyUsedByLifetimeMarkersOrDroppableInsts(BCI)) diff --git a/llvm/test/Transforms/Coroutines/coro-alloca-outside-frame.ll b/llvm/test/Transforms/Coroutines/coro-alloca-outside-frame.ll index ac6a5752438ce..e227acb082d11 100644 --- a/llvm/test/Transforms/Coroutines/coro-alloca-outside-frame.ll +++ b/llvm/test/Transforms/Coroutines/coro-alloca-outside-frame.ll @@ -2,10 +2,29 @@ ; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S -o %t.ll ; RUN: FileCheck --input-file=%t.ll %s -define ptr @f(i1 %n) presplitcoroutine { +; %y and %alias_phi would all go to the frame, but not %x +; CHECK: %g.Frame = type { ptr, ptr, i64, ptr, i1 } + +; CHECK-LABEL: @g( +; CHECK: %x = alloca i64, align 8 +; CHECK-NOT: %x.reload.addr = getelementptr inbounds %g.Frame, ptr %hdl, i32 0, i32 2 +; CHECK: %y.reload.addr = getelementptr inbounds %g.Frame, ptr %hdl, i32 0, i32 2 +; CHECK: %alias_phi = phi ptr [ %y.reload.addr, %merge.from.flag_false ], [ %x, %entry ] + + +; The deprecated !coro.outside.frame metadata is parsed but doesn't do anything. +define ptr @f() { entry: %x = alloca i64, !coro.outside.frame !{} + ret ptr %x +} + + +define ptr @g(i1 %n) presplitcoroutine { +entry: + %x = alloca i64 %y = alloca i64 + call void @llvm.coro.outside.frame(ptr %x) %id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null) %size = call i32 @llvm.coro.size.i32() %alloc = call ptr @malloc(i32 %size) @@ -37,13 +56,6 @@ suspend: ret ptr %hdl } -; %y and %alias_phi would all go to the frame, but not %x -; CHECK: %f.Frame = type { ptr, ptr, i64, ptr, i1 } -; CHECK-LABEL: @f( -; CHECK: %x = alloca i64, align 8, !coro.outside.frame !0 -; CHECK-NOT: %x.reload.addr = getelementptr inbounds %f.Frame, ptr %hdl, i32 0, i32 2 -; CHECK: %y.reload.addr = getelementptr inbounds %f.Frame, ptr %hdl, i32 0, i32 2 -; CHECK: %alias_phi = phi ptr [ %y.reload.addr, %merge.from.flag_false ], [ %x, %entry ] declare ptr @llvm.coro.free(token, ptr) declare i32 @llvm.coro.size.i32() @@ -58,4 +70,4 @@ declare i1 @llvm.coro.end(ptr, i1, token) declare void @print(ptr) declare noalias ptr @malloc(i32) -declare void @free(ptr) \ No newline at end of file +declare void @free(ptr) diff --git a/llvm/test/Transforms/Mem2Reg/ignore-coro-outside-frame.ll b/llvm/test/Transforms/Mem2Reg/ignore-coro-outside-frame.ll new file mode 100644 index 0000000000000..ec9309503109d --- /dev/null +++ b/llvm/test/Transforms/Mem2Reg/ignore-coro-outside-frame.ll @@ -0,0 +1,13 @@ +; RUN: opt -passes=mem2reg -S -o - < %s | FileCheck %s + +declare void @llvm.coro.outside.frame(ptr) + +define void @test() { +; CHECK: test +; CHECK-NOT: alloca +; CHECK-NOT: call void @llvm.coro.outside.frame + %A = alloca i32 + call void @llvm.coro.outside.frame(ptr %A) + store i32 1, ptr %A + ret void +} `````````` </details> https://github.com/llvm/llvm-project/pull/129255 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
