[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

[Matt Arsenault via Phabricator via cfe-commits]

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