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

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9461-9463 + bool CorrectSqrt = CGM.getLangOpts().OpenCL + ? CGM.getCodeGenOpts().OpenCLCorrectlyRoundedDivSqrt + : CGM.getCodeGenOpts().HIPCorrectlyRoundedD

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

arsenm added a comment. We should just do this now. clang shouldn't have to dig around on disk to emit a constant definition for a constant it already knows, and we have a clear path to removing these globals altogether. I have adequate patches to completely delete `__oclc_daz_opt` today. `__oc

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

arsenm added a comment. In D130096#3850708 , @b-sumner wrote: >> Different functions providing different behaviors can be handled at link >> time like any other function, instead of the same functions providing >> different behaviors per translation uni

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

b-sumner added a comment. > Different functions providing different behaviors can be handled at link time > like any other function, instead of the same functions providing different > behaviors per translation unit and requires cloning. The current scheme > transfers complexity from the device

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

jhuber6 added a comment. In D130096#3850628 , @arsenm wrote: > In D130096#3850550 , @b-sumner > wrote: > >> There's the "small matter" of implementing the new device library functions. >> Why is all that more l

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

arsenm added a comment. In D130096#3850550 , @b-sumner wrote: > There's the "small matter" of implementing the new device library functions. > Why is all that more likeable than two kinds of control constants? Different functions providing different be

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

b-sumner added a comment. In D130096#3850473 , @arsenm wrote: > In D130096#3850472 , @jhuber6 wrote: > >> I don't like the fact that we need to have two different kinds of control >> constants, one per-TU and oth

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

arsenm added a comment. In D130096#3850472 , @jhuber6 wrote: > I don't like the fact that we need to have two different kinds of control > constants, one per-TU and others per-link job. I'm wondering how difficult it > would be to make the fast versions

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

jhuber6 added a comment. I don't like the fact that we need to have two different kinds of control constants, one per-TU and others per-link job. I'm wondering how difficult it would be to make the fast versions of the math calls use different entry points. That way we could handle this in the

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

yaxunl added inline comments. Comment at: clang/test/CodeGen/amdgcn-control-constants.c:8 + +// GFX90A: @__oclc_daz_opt = linkonce_odr hidden local_unnamed_addr addrspace(4) constant i8 0, align 1 +// GFX90A: @__oclc_wavefrontsize64 = linkonce_odr hidden local_unnamed_addr addr

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

jhuber6 added inline comments. Comment at: clang/test/CodeGen/amdgcn-control-constants.c:8 + +// GFX90A: @__oclc_daz_opt = linkonce_odr hidden local_unnamed_addr addrspace(4) constant i8 0, align 1 +// GFX90A: @__oclc_wavefrontsize64 = linkonce_odr hidden local_unnamed_addr add

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

yaxunl added inline comments. Comment at: clang/test/CodeGen/amdgcn-control-constants.c:8 + +// GFX90A: @__oclc_daz_opt = linkonce_odr hidden local_unnamed_addr addrspace(4) constant i8 0, align 1 +// GFX90A: @__oclc_wavefrontsize64 = linkonce_odr hidden local_unnamed_addr addr

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

jhuber6 updated this revision to Diff 464767. jhuber6 added a comment. Moving test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 Files: clang/lib/CodeGen/CodeGenAction.cpp clang/lib/CodeGen/CodeGenM

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

yaxunl added inline comments. Comment at: clang/test/CodeGen/amdgcn-link-control-constants.c:2-3 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --global-value-regex "__oclc_

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

JonChesterfield added a comment. In D130096#3816149 , @arsenm wrote: > I'd prefer to avoid spreading special treatment of the device libraries into > the backend. The contract is poorly defined and spread around too much as it > is

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

jhuber6 added inline comments. Comment at: clang/test/CodeGen/amdgcn-link-control-constants.c:2-3 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --global-value-regex "__oclc

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

jhuber6 added inline comments. Comment at: clang/test/CodeGen/amdgcn-link-control-constants.c:2-3 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --global-value-regex "__oclc

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

yaxunl added inline comments. Comment at: clang/test/CodeGen/amdgcn-link-control-constants.c:2-3 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --global-value-regex "__oclc_

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

jhuber6 added a comment. ping 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

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

jhuber6 updated this revision to Diff 463042. jhuber6 added a comment. Adding test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 Files: clang/lib/CodeGen/CodeGenAction.cpp clang/lib/CodeGen/CodeGenM

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

yaxunl added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:299-308 + if (!LinkModules.empty() && Gen->CGM().getTriple().isAMDGCN() && + !Gen->CGM().getLangOpts().GPURelocatableDeviceCode) { +const StringRef GVS[] = {"__oclc_daz_opt", "__oc

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

arsenm added a comment. In D130096#3815529 , @jhuber6 wrote: > The best solution would be to handle these per-TU variables in the backend. > Or maybe even all of these could be placed in the backend where the code > paths that currently require a contro

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

jhuber6 updated this revision to Diff 462948. jhuber6 added a comment. Adding an extra check in `CodeGenAction.cpp` that forcibly internalizes these if we link in any modules in RDC mode. This is a considerable hack, but should solve the problem. It's not a great solution, so let me know if you

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

yaxunl added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468 + // Control constants for math operations. + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); jhuber6 wrote:

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

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468 + // Control constants for math operations. + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); yaxunl wrote:

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

yaxunl added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468 + // Control constants for math operations. + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); jhuber6 wrote:

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

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468-9472 + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); + AddGlobal("__oclc_finite_only_opt", FiniteOnly || RelaxedMath, /*Si

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

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468 + // Control constants for math operations. + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); yaxunl wrote:

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

yaxunl added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9468-9472 + AddGlobal("__oclc_wavefrontsize64", Wavefront64, /*Size=*/8); + AddGlobal("__oclc_daz_opt", DenormAreZero, /*Size=*/8); + AddGlobal("__oclc_finite_only_opt", FiniteOnly || RelaxedMath, /*Siz

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

jhuber6 updated this revision to Diff 460812. jhuber6 added a comment. Addressing comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/lib/CodeGen

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

jhuber6 marked 3 inline comments as done. jhuber6 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9449-9450 + !(Features & llvm::AMDGPU::FEATURE_WAVE32) || + llvm::is_contained(CGM.getTarget().getTargetOpts().FeaturesAsWritten, +

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

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9449-9450 + !(Features & llvm::AMDGPU::FEATURE_WAVE32) || + llvm::is_contained(CGM.getTarget().getTargetOpts().FeaturesAsWritten, + "+wavefrontsize64"); +

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

jhuber6 updated this revision to Diff 458186. jhuber6 added a comment. Changing to `linkonce` linkage. According to the LLVM spec this should have the expected behaviour where a single definition is kept at link-time for each module. I tested this with a sample `HIP` program and it had the desired

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

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

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

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

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

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

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

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

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

jhuber6 updated this revision to Diff 456520. jhuber6 added a comment. Remove unused code gen option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130096/new/ https://reviews.llvm.org/D130096 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/l

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

jdoerfert added a comment. If you want to overwrite them, weak/linkonce will work (no _odr). Private/internal will not be overwritten but existing uses will keep the private/internal version, IIRC. I assume you want the former. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jhuber6 updated this revision to Diff 456450. jhuber6 added a comment. Changing to private linkage. For OpenMP we could either make this use `weak_odr` so we have a single definition surviving until link time for us to use. Or we could change OpenMP to link in the bitcode libraries per-TU via `-m

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

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

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

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, yaxunl wrote: > jhuber

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

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

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

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

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

jhuber6 updated this revision to Diff 456441. jhuber6 added a comment. Updating. I realized all of the math-related ones are already covered by driver options for AMDGPU passing the appropriate fp contract to the frontend. This patch gets rid of most of that handling and just uses those directly

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

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, This does not support

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

jhuber6 updated this revision to Diff 453021. jhuber6 added a comment. Adjusting, adding code generation options for the other constants and changing to use linkonce ODR linkage. I attempted to follow Jon's suggestion and group it with the existing code. but all the existing handling for this o

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

jhuber6 added a comment. In D130096#3666155 , @yaxunl wrote: > The current patch does not consider HIP/OpenCL compile options, therefore the > value of these variables are not correct for OpenCL/HIP. They need to be > overridden by the variables with th

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

yaxunl added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9480 + AddGlobal("__oclc_ISA_version", Minor + Major * 1000, 32); + AddGlobal("__oclc_ABI_version", 400, 32); +} jhuber6 wrote: > yaxunl wrote: > > should be determined by the code objec

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

yaxunl added a comment. In D130096#3663411 , @arsenm wrote: > In D130096#3663398 , @jhuber6 wrote: > >> In D130096#3663295 , @yaxunl wrote: >> >>> There is no constant pro

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

jhuber6 added a comment. In D130096#3663062 , @JonChesterfield wrote: > A safer bet is to use the current control flow that links in specific bitcode > files, but create the global directly instead of linking in the file. That'll > give us zero semanti

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

jhuber6 added a comment. In D130096#3663411 , @arsenm wrote: > In D130096#3663398 , @jhuber6 wrote: > >> In D130096#3663295 , @yaxunl wrote: >> >>> There is no constant pr

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

arsenm added a comment. In D130096#3663398 , @jhuber6 wrote: > In D130096#3663295 , @yaxunl wrote: > >> There is no constant propagation for globals with weak linage, right? >> Otherwise, it won't work. My concer

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

jhuber6 added a comment. In D130096#3663295 , @yaxunl wrote: > There is no constant propagation for globals with weak linage, right? > Otherwise, it won't work. My concern is that there may be optimization passes > which do not respect the weak linkage

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

jhuber6 marked an inline comment as done. jhuber6 added a comment. In D130096#3663295 , @yaxunl wrote: > There is no constant propagation for globals with weak linage, right? > Otherwise, it won't work. My concern is that there may be optimization passes

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

yaxunl added a comment. There is no constant propagation for globals with weak linage, right? Otherwise, it won't work. My concern is that there may be optimization passes which do not respect the weak linkage and uses the incorrect default value for OpenCL or HIP. Therefore I am not very confi

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

jhuber6 added a comment. In D130096#3663062 , @JonChesterfield wrote: > A safer bet is to use the current control flow that links in specific bitcode > files, but create the global directly instead of linking in the file. That'll > give us zero semanti

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

JonChesterfield added a comment. A safer bet is to use the current control flow that links in specific bitcode files, but create the global directly instead of linking in the file. That'll give us zero semantic change and a clang that ignores those bitcode files if present. Repository: rG L

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

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9456 +llvm::ConstantInt::get(Type, Value), Name, nullptr, +llvm::GlobalValue::ThreadLocalMode::NotThreadLocal, 4); +GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Local); --

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

jhuber6 added a comment. In D130096#3663010 , @JonChesterfield wrote: > Tagging Brian as the code owner of rocm device libs - emitting these in clang > would simplify that library. > > Currently clang reads these commandline flags and conditionally link

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

arsenm added a comment. I've thought that directly emitting these constants would be better. This will also make it so you can't try to continue using llvm-link for these libraries, which is a plus since it doesn't have the same necessary attribute propagation clang does when linking these Re

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

JonChesterfield added a reviewer: b-sumner. JonChesterfield added a comment. Tagging Brian as the code owner of rocm device libs - emitting these in clang would simplify that library. Currently clang reads these commandline flags and conditionally links in bitcode files to introduce these symbo

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

jhuber6 added a comment. Let me know if I should move this code somewhere else, or if there are problems. One change I made is that the constant is `weak_odr` and `hidden` instead of `linkonce_odr` and `protected`. This is so this constant is alive until link time, AMDGPU pretty much always use

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

jhuber6 created this revision. jhuber6 added reviewers: JonChesterfield, yaxunl, saiislam, arsenm, carlo.bertolli, MaskRay, jdoerfert, tianshilei1992. Herald added subscribers: kosarev, StephenFan, t-tye, tpr, dstuttard, jvesely, kzhuravl. Herald added a project: All. jhuber6 requested review of