[clang] hipcc/ld.lld unable to link separable compilation when dynamic librar… (PR #169551)

[David Salinas via cfe-commits]

david-salinas wrote:

> > > Is the failure related to this change?
> > 
> > 
> > Yes, because I moved the "if (!isHidden)" up to the same if statement as "if (!isUndefined)". But now we skip the "continue" at around line 256 (
> > https://github.com/llvm/llvm-project/blob/4a48740831d0f0779780e0bea64ec4a16d9f6d97/clang/lib/Driver/ToolChains/HIPUtility.cpp#L256
> > 
> > ), and end up adding the symbols to the GPUBinHandleSymbols vector in the subsequent "if" statement for undefined symbols. To minimize the changes needed, and to reduce the impact on existing behaviour, I'm going to just leave the check for "!isHidden" in the two separate in if stmts.
> 
> Another possibility is the following, though it does increase the number of changes.
> 
> ```c++
>       // Add undefined symbols if they are not in the defined sets
>       if (isUndefined) {
>         if (isFatBinSymbol &&
>             DefinedFatBinSymbols.find(Name) == DefinedFatBinSymbols.end())
>           FatBinSymbols.insert(Name.str());
>         else if (isGPUBinHandleSymbol && DefinedGPUBinHandleSymbols.find(Name) ==
>                                              DefinedGPUBinHandleSymbols.end())
>           GPUBinHandleSymbols.insert(Name.str());
>         continue;
>       }
> 
>       // Ignore hidden defined symbols
>       if (isHidden)
>         continue;
> 
>       // Handling for non-hidden defined symbols
>       if (isFatBinSymbol) {
>         DefinedFatBinSymbols.insert(Name.str());
>         FatBinSymbols.erase(Name.str());
>       } else if (isGPUBinHandleSymbol) {
>         DefinedGPUBinHandleSymbols.insert(Name.str());
>         GPUBinHandleSymbols.erase(Name.str());
>       }
> ```
> 
> Can there ever be hidden undefined fatbin or gpubin handle symbols? The current code would still add these as a primary or alias symbols, as best as I can tell. If they should either be ignored or cannot exist, the `if (isHidden)` condition can be lifted above `if (isUndefined)`, which I think makes the code a tad clearer.
> 
> Unrelatedly, do you know how I can get my AMD email to be used for the `Co-authored-by` line when that gets appended?

Hi @omor1 yes, there can be hidden and undefined fatbin or gpubin symbols, the hip-partial-link LIT test tests that actually.  I had something like this code (above) at one point, but though it would be too much change, and there were a couple of cases that I missed.  But I just tried this change and it seems to pass all LIT tests and the original problem in the ticket.  I'm not a big fan of the "continues" stylistically, but this function already does it above.  So I'm inclined to use this change.  :-)  I'll push this change in a new commit for this PR.  Thanks!

https://github.com/llvm/llvm-project/pull/169551