[PATCH] D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:894
+                                FrameDataInfo &FrameData) {
+  if (!llvm::count_if(F.getParent()->debug_compile_units(), [](auto *Iter) {
+        return dwarf::isCPlusPlus(
----------------
dblaikie wrote:
> lxfind wrote:
> > Does this mean that if there is any C++ files, we don't build debug info?
> aside, I guess this should probably be using `llvm::none_of` - it'll shortcircuit (won't keep testing every CU if it finds one that is C++) and is more clear about the semantic goal of the test compared to the !count current phrasing.
> 
> But also: Why is the code searching all the CUs? Wouldn't it be better to test the CU of the Function being passed in only? Then this code won't behave differently under LTO?
@dblaikie Thanks for reminding! Previously I am not so familiar with the API system for debug. It makes more sense to test the CU for the Function only.

This wouldn't behave differently under LTO since the related intrinsic would be lowered after we construct the coroutine frame.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:907
+  assert(PromiseAlloca &&
+         "Coroutine with switch ABI should own Promise alloca");
+
----------------
lxfind wrote:
> Sorry I didn't mean to ask for this change, but more of curiosity, I see that in many places we check whether PromiseAlloca is nullptr under switch-lowering. So I wonder if there exists legit cases where PromiseAloca is null?
To my knowledge, there wouldn't be the cases the PromiseAlloca is null even the promise_type in C++ are an empty class. I guess other  codes are also workarounds to the test cases. We could clean them in future.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1474
   LLVMContext &C = CB->getContext();
   IRBuilder<> Builder(CB->getNextNode());
   StructType *FrameTy = Shape.FrameTy;
----------------
lxfind wrote:
> ChuanqiXu wrote:
> > lxfind wrote:
> > > Builder should no longer be initialized with CB->getNextNode, but just the context
> > `IRBuilder<> Builder(CB->getNextNode())` tells where builder should insert instructions. I think it is not redundant.
> If you look closely, this insert position is never used. In fact, it's confusing to set the insert position to the next instruction of CoroBegin after this patch, because now the next instruction of CoroBegin is the bitcast of the frame pointer. You don't want to insert anything before the frame pointer construction anyway.
Done. You are right.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99179/new/

https://reviews.llvm.org/D99179