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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:345
+    assert((LayoutIndexUpdateStarted || FieldAlignMap.count(V) == 0));
+    FieldAlignMap[V] = Align;
+  }
----------------
`FieldAlignMap.insert({V, Align})`?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:766
+    std::string TypeName =
+        "int" + std::to_string(cast<IntegerType>(Ty)->getBitWidth());
+    auto *MDName = MDString::get(Ty->getContext(), TypeName.c_str());
----------------
string stream?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:767
+        "int" + std::to_string(cast<IntegerType>(Ty)->getBitWidth());
+    auto *MDName = MDString::get(Ty->getContext(), TypeName.c_str());
+    return MDName->getString();
----------------
We should use the constructor that takes a StringRef not the one that takes a char*


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:843
+    SmallVector<Metadata *, 16> Elements;
+    for (unsigned I = 0; I < StructTy->getNumElements(); I++) {
+      DIType *DITy = solveDIType(Builder, StructTy->getElementType(I), Layout,
----------------
could this be written as a range-based for?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:860
+    RetType = Builder.createBasicType(
+        Name.str() + "_" + std::to_string(Layout.getTypeSizeInBits(Ty)),
+        Layout.getTypeSizeInBits(Ty), dwarf::DW_ATE_address);
----------------
Perhaps use one of the llvm string stream classes here to minimize all the std::string copying?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:864
+
+  DITypeCache[Ty] = RetType;
+  return RetType;
----------------
I would also use insert here to make it clearer that we are adding a new entry and not overwriting an existing one


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:919
+  unsigned ResumeIndex = coro::Shape::SwitchFieldIndex::Resume,
+           DestroyIndex = coro::Shape::SwitchFieldIndex::Destroy,
+           IndexIndex = Shape.SwitchLowering.IndexField;
----------------
unsigned ... ;


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:923
+  DenseMap<unsigned, StringRef> NameCache;
+  NameCache.insert(std::make_pair(ResumeIndex, "__resume_fn"));
+  NameCache.insert(std::make_pair(DestroyIndex, "__destroy_fn"));
----------------
NameCache.insert({ResumeIndex, "__resume_fn"});



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:954
+
+    NameCache.insert(std::make_pair(Index, DIVarCache[V]->getName()));
+    TyCache.insert(std::make_pair(Index, DIVarCache[V]->getType()));
----------------
NameCache.insert({Index, DIVarCache[V]->getName())});


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:961
+  // The Align and Offset of Resume function and Destroy function are fixed.
+  OffsetCache[ResumeIndex] = std::make_pair(8, 0);
+  OffsetCache[DestroyIndex] = std::make_pair(8, 8);
----------------
OffsetCache.insert({ResumeIndex, {8, 0}};


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

https://reviews.llvm.org/D99179