bruno added a comment.
Hi Xun, great to see more improvements in this area.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221
CGF.EmitBlock(RealSuspendBlock);
+ } else if (ForcestackStart) {
+ Builder.CreateCall(
----------------
ChuanqiXu wrote:
> lxfind wrote:
> > ChuanqiXu wrote:
> > > ChuanqiXu wrote:
> > > > can we rewrite it into:
> > > > ```
> > > > else if (SuspendRet != nullptr && SuspendRet->getType()->isClassType())
> > > > {
> > > > // generate:
> > > > // llvm.coro.forcestack(SuspendRet)
> > > > }
> > > > ```
> > > Sorry I find we can't did it directly. As you said, we need to traverse
> > > down SuspendRet. And I still think we should did it only at CodeGen part
> > > since it looks not so hard. I guess we could make it in above 10~15 lines
> > > of codes.
> > Traversing down AST isn't the hard part. The hard part is to search the
> > emitted IR, and look for the temporary alloca used to store the returned
> > handle.
> Yes, I get your point. If we want to traverse the emitted IR, we could only
> search for the use-chain backward, which is also very odd. Let's see if there
> is other ways to modify the ASTNodes to make it more naturally.
I'm curious whether did you consider annotating instructions with some new
custom metadata instead of using intrinsics? If so, what would be the tradeoff?
For example, if you could conditionally attach metadata some "begin" metadata
here:
`auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});`
and "end" metadata here:
`auto *SuspendResult = Builder.CreateCall(CoroSuspend, {SaveCall,
Builder.getInt1(IsFinalSuspend)});`
================
Comment at: clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp:53
-// check that the lifetime of the coroutine handle used to obtain the address
is contained within single basic block, and hence does not live across
suspension points.
-// CHECK-LABEL: final.suspend:
-// CHECK: %[[PTR1:.+]] = bitcast
%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
%[[ADDR_TMP:.+]] to i8*
-// CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[PTR1]])
-// CHECK: call i8*
@{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
{{[^,]*}} %[[ADDR_TMP]])
-// CHECK-NEXT: %[[PTR2:.+]] = bitcast
%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* %[[ADDR_TMP]]
to i8*
-// CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[PTR2]])
+// CHECK-LABEL: |-FunctionDecl {{.*}} foo 'detached_task ()'
+// first ExprWithCleanups is the initial await
----------------
Nice tests. The codegen should live in a different file from the AST dump one,
you can put the later in `test/clang/SemaCXX` or `tes/clang/AST`.
================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1308
+// Like the sideeffect intrinsic defined above, this intrinsic is treated by
the
+// optimizer as having opaque side effects so that it won't be get rid of or
moved
// out of the block it probes.
----------------
This change seems unrelated to this patch.
================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2083
+ ForceStackList ForceStacks;
+ for (auto &I : instructions(F))
+ if (auto *II = dyn_cast<IntrinsicInst>(&I))
----------------
`collectForceStacks` is only called once from a function that already traverses
all instructions, can you take advantage of that to collect
`llvm::Intrinsic::coro_forcestack_begin/end`?
================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2085
+ if (auto *II = dyn_cast<IntrinsicInst>(&I))
+ if (II->getIntrinsicID() == llvm::Intrinsic::coro_forcestack_begin) {
+ assert(II->getNumUses() == 1 &&
----------------
Do such intrinsics never get removed? What happens when this hits a backend?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98638/new/
https://reviews.llvm.org/D98638
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
