[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:371-372
   DenseMap<Value *, uint32_t> FieldIndexMap;
+  DenseMap<Value *, uint64_t> FieldAlignMap;
+  DenseMap<Value *, uint64_t> FieldOffsetMap;
 };
----------------
lxfind wrote:
> Please add comment to the fields
Add comments for when would these fields be initialized. So others could use them after that. I didn't constraint the usage in the comments since if we use them in other places, the comments may be inconsistent with the code.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:901
+
+  AllocaInst *PromiseAlloca = Shape.getPromiseAlloca();
+  if (!PromiseAlloca)
----------------
lxfind wrote:
> I am always curious, when would PromiseAlloca to not be present?
Done. It would happen in test cases. I should had edit the test cases and add an assertion here.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1474
   LLVMContext &C = CB->getContext();
   IRBuilder<> Builder(CB->getNextNode());
   StructType *FrameTy = Shape.FrameTy;
----------------
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.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroInternal.h:136-137
     unsigned IndexField;
+    unsigned IndexAlign;
+    unsigned IndexOffset;
     bool HasFinalSuspend;
----------------
lxfind wrote:
> Comment about the purpose
I think the meaning of the new added fields are clear from their name. And for the purpose, I wouldn't like to constrain it in the comments. Because if we use them in other places in the future, it is very easy to become inconsistent.


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

https://reviews.llvm.org/D99179