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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D99179#2688242 <https://reviews.llvm.org/D99179#2688242>, @ChuanqiXu wrote:

> In D99179#2688049 <https://reviews.llvm.org/D99179#2688049>, @dblaikie wrote:
>
>> Something @probinson mentioned in the llvm-dev thread is that any debug info this creates should be sure that it doesn't produce duplicate unique entities - if it has to create any support types. Does the code correctly only produce one of them, even if there are multiple coroutines being lowered?
>
> Thanks for reminding! I don't know why I don't get the responding from @probinson even in the spam list. And I find it here: https://groups.google.com/g/llvm-dev/c/hfZ6z_MJFOw.
> It looks like it wouldn't create redundant base types. But it would still create one coroutine frame type for each coroutine. It makes sense because the layout of coroutine is different with each other (Although it could be the same, as the updated test case tells).
> And I didn't do type cache in the module level actually. So I guess it is handled by the infrastructure of the DI system.

Hmm - does the test case cover this situation where there are two coroutine frames independently constructed/lowered, but they share basic types? My first look at some of the basic types created only shows them being used in one frame.

Also, can this end up creating multiple types and variables with the same name in the same scope? If so, maybe those types and variables (the coro_frame types and variables specifically, from what I can see) should be unnamed instead of duplicately named? (or their names should somehow be uniqued, but I imagine that's non-trivial, having to look to see what coroutines have been lowered already/what names are already used)



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:752-758
+    for (auto *DDI : FindDbgDeclareUses(V)) {
+      if (DDI->getExpression()->getNumElements() != 0)
+        continue;
+
+      DIVarCache.insert(std::make_pair(V, DDI->getVariable()));
+      break;
+    }
----------------
I'd probably write this with llvm::find_if, if I'm understanding the code correctly it would be equival/ent to this:

```
auto DDIs = FindDbgDeclareUses(V);
auto I = llvm::find_if(DDIs, [](DbgDeclareInst* DDI) { return DDI->getExpression()->getNumElements() == 0; });
if (I != DDIs.end())
  DIVarCache.insert({V, DDI->getVariable()});
```


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:773-778
+    if (Ty->isFloatTy())
+      return "__float_";
+    else if (Ty->isDoubleTy())
+      return "__double_";
+    else
+      return "__floating_type_";
----------------
Skip else after return ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:814-815
+                           DenseMap<Type *, DIType *> &DITypeCache) {
+  if (DITypeCache.find(Ty) != DITypeCache.end())
+    return DITypeCache[Ty];
+
----------------
This does two map lookups - best to do it with one by reusing the found iterator:
```
if (DIType *DT = DITypeCache.lookup(Ty))
  return DT;
```

(assuming there aren't any null values in the cache)


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

https://reviews.llvm.org/D99179