arsenm 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:
> > 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
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
