jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.
LG, with or without the -lm lookup
================
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)) {
----------------
arsenm wrote:
> jhuber6 wrote:
> > 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.
> It can be subtley wrong, and also pushes reliance on setting the global
> subtarget. It's best to ensure the default attributes are propagated to the
> device libraries
We will build a better solution for this soon, for now this is "good enough".
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
