jdoerfert added a subscriber: arsenm.
jdoerfert 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)) {
----------------
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?
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8205
+ if (llvm::find(LibraryArgs, "m") == LibraryArgs.end() && !D.CCCIsCXX())
+ continue;
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119841/new/
https://reviews.llvm.org/D119841
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
