[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper
Joseph Huber via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 15 11:58:18 PST 2022
jhuber6 added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8195
+ // Get the AMDGPU math libraries.
+ // FIXME: This method is bad, remove once AMDGPU has a proper math library.
+ for (auto &I : llvm::make_range(OpenMPTCRange.first, OpenMPTCRange.second)) {
----------------
jdoerfert wrote:
> jhuber6 wrote:
> > jdoerfert wrote:
> > > Can you elaborate on this comment, what's bad, how would the better version look
> > It's explained in more detail where this is done for the AMDGPU ToolChain, e.g.
> > ```
> > // This is not certain to work. The device libs added here, and passed to
> > // llvm-link, are missing attributes that they expect to be inserted when
> > // passed to mlink-builtin-bitcode. The amdgpu backend does not generate
> > // conservatively correct code when attributes are missing, so this may
> > // be the root cause of miscompilations. Passing via mlink-builtin-bitcode
> > // ultimately hits CodeGenModule::addDefaultFunctionDefinitionAttributes
> > // on each function, see D28538 for context.
> > // Potential workarounds:
> > // - unconditionally link all of the device libs to every translation
> > // unit in clang via mlink-builtin-bitcode
> > // - build a libm bitcode file as part of the DeviceRTL and explictly
> > // mlink-builtin-bitcode the rocm device libs components at build time
> > // - drop this llvm-link fork in favour or some calls into LLVM, chosen
> > // to do basically the same work as llvm-link but with that call first
> > // - write an opt pass that sets that on every function it sees and pipe
> > // the device-libs bitcode through that on the way to this llvm-link
> > ```
> > Should I copy the gist here?
> Is it still relevant? We don't use llvm-link here, do we?
>
> @arsenm, the backend is (almost) OK with the lack of attributes, is it not?
Linking is done using LTO now, I don't know exactly how they merge bitcode compared to llvm-link but I'm assuming it's similar.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8205
+ if (llvm::find(LibraryArgs, "m") == LibraryArgs.end() && !D.CCCIsCXX())
+ continue;
+
----------------
jdoerfert wrote:
> I'd switch the conditions.
>
> More importantly, does this require that the user passes -lm to the linker invocation? I'm not convinced we should not always link these in.
Yes, would save some time assuming most codes are C++
So I figured I'd copy the same semantics of how `-lm` works where you need to specify it for C but not C++. We could just pass this in all the time, but since linking it in currently required `-lm` I copied that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119841/new/
https://reviews.llvm.org/D119841
More information about the cfe-commits
mailing list