[PATCH] D99693: Update the linkage name of coro-split functions where applicable

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:866
+    // linkage name and asserts that they are identical.
+    if (!SP->getDeclaration())
+      SP->replaceLinkageName(MDString::get(Context, NewF->getName()));
----------------
aprantl wrote:
> ChuanqiXu wrote:
> > aprantl wrote:
> > > kastiglione wrote:
> > > > Is `!SP->getDeclaration()` the implicit check for swift but not C++?
> > > Yes. If you have a top-level C++ coroutine — and I have no idea if that's a thing, you would get the "real" linkage name.
> > Does this mean the DISubprogram for every C++ coroutine did contain a declaration? May I ask where the constraint is?
> The problem we're working around here is (as far as I understand) a limitation of LLVM itself. The `distinct DISubprogram`s for C++ *methods* always have a a `declaration:` pointing into the uniqued `DISubprogram` in the type system that contains their abstract declaration. This is roughly equivalent to the `DW_AT_specification`  in the DWARF for the concrete method pointing to the abstract declaration that holds all the shared attributes.
> 
> When emitting a `DW_AT_specification` attribute AsmPrinter asserts that all attributes in the `distinct DISubprogram` are equivalent to the ones in the abstract `DISubprogram` declaration. That's why we can't modify the linkage name for a DISubprogram that had a `declaration:` specified, because it would clash with the linkage name in the specification.
> 
> ```
> 0x200:
> DW_TAG_subprogram // abstract decl
>   DW_AT_linkage_name (foo)
>   DW_TAG_formal_paramater ...
>   ...
> 
> DW_TAG_subprogram // concrete instance 1
>   DW_AT_low_pc (0x1000)
>   ...
>   DW_AT_specification (0x200) // points to decl for common  bits
> 
> DW_TAG_subprogram // concrete instance 2
>   DW_AT_low_pc (0x1500)
>   ...
>   DW_AT_specification (0x200) // points to decl for common  bits
> ```
Got it. I am prefer to add guard if or assertion to make the semantics explicitly. Like:
```
-     if (!SP->getDeclaration())
+    if (!SP->getDeclaration() && Shape.ABI == coro::ABI::Retcon)
```
or:
```
+   assert(Shape.ABI == coro::ABI::Retcon && "We should only update a linkageName for Swift.");
     SP->replaceLinkageName(MDString::get(Context, NewF->getName()));
```
I am not sure about the constraint `Shape.ABI == coro::ABI::Retcon` since MLIR would also use this code.


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

https://reviews.llvm.org/D99693