yaxunl 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, ---------------- jhuber6 wrote: > 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. > 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. On second thoughts, the idea for letting clang to emit these control variables might not work for HIP and OpenCL. The reason is that to support per-TU control variables, these variables need to be internal or private linkage, however, that means they cannot be used by other device library functions which are expecting non-internal linkage for them. Those device library functions will end up using control variables from device library bitcode any way. For OpenMP, it may be necessary to emit them as linkonce_odr, otherwise device library functions may not find them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
