[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9436
+        CGM.getModule(), Type, true,
+        llvm::GlobalValue::LinkageTypes::LinkOnceODRLinkage,
+        llvm::ConstantInt::get(Type, Value), Name, nullptr,
----------------
yaxunl wrote:
> yaxunl wrote:
> > jhuber6 wrote:
> > > yaxunl wrote:
> > > > This does not support per-TU control variables. Probably should use internal linkage.
> > > The AMDGPU device libraries use `linkone_odr` so I figured it was the most appropriate here. It should mean that we can have multiple identical definitions and they don't clash. There's also no requirement for these to be emitted as symbols AFAIK.
> > > The AMDGPU device libraries use `linkone_odr` so I figured it was the most appropriate here. It should mean that we can have multiple identical definitions and they don't clash. There's also no requirement for these to be emitted as symbols AFAIK.
> > 
> > clang uses  -mlink-builtin-bitcode to link these device libraries for HIP and OpenCL. Then the linkage of these variables becomes internal linkage. That's why it works for per-TU control.
> > > The AMDGPU device libraries use `linkone_odr` so I figured it was the most appropriate here. It should mean that we can have multiple identical definitions and they don't clash. There's also no requirement for these to be emitted as symbols AFAIK.
> > 
> > clang uses  -mlink-builtin-bitcode to link these device libraries for HIP and OpenCL. Then the linkage of these variables becomes internal linkage. That's why it works for per-TU control.
> 
> You may let HIP and OpenCL use internal linkage and C/C++/OpenMP use linkonce_odr since only HIP and OpenCL toolchain use -mlink-builtin-bitcode to link these device libraries 
I see, `linkonce_odr` implies that these should all have the same value which isn't necessarily true after linking. I'll change it to use private linkage.

OpenMP right now links everything late which means that we don't allow these to be defined differently per-TU. This may be incorrect given this new method as each TU will have different things set. I can change OpenMP to use the `mlink` method after this patch which may be more strictly correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130096