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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
