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

Wed Feb 16 06:20:33 PST 2022

JonChesterfield added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8205
+    if (llvm::find(LibraryArgs, "m") == LibraryArgs.end() && !D.CCCIsCXX())
+      continue;
+
----------------
jdoerfert wrote:
> JonChesterfield wrote:
> > jhuber6 wrote:
> > > 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.
> > re: always linking these libraries in, regardless of -lm, it's probably better to link them by default and effectively ignore -lm.
> > 
> > I'd like to keep it guarded by -nogpulib or similar so that people can still opt out of the rocm device library stack entirely.
> Opting out of "optional" gpu libraries is fine. I want to avoid that one needs to add linking flags etc. for OpenMP. We link libdevice by default too, so this is no different (as both contain math function impl etc.)
I guess that hangs on whether libm is optional or not.

I am reluctant to tie every openmp execution to the rocm libdevice as it frequently fails to build with llvm trunk and I'm nervy about linking IR built by the rocm clang with IR generated from trunk. It mostly seems to work for rocm sufficiently similar in age to trunk but periodically the abi or intrinsic set diverges. 

Maybe our working assumption should be that everyone using openmp wants libm so rocm breaking trunk openmp has to be resolved each time it happens anyway. If so, there's a design decision to be made about how to manage that dependency.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119841/new/

https://reviews.llvm.org/D119841