[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm closed this revision. arsenm added a comment. f4bcd7f598331457cfe74e459b489d4098369511 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

sameerds added a comment. And note that the change description is written in a first-person train of thought. Please do rewrite it! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 ___ cfe-commits

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

sameerds accepted this revision. sameerds added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:353 + if (HaveWave32 && HaveWave64) { +Diags.Report(diag::err_invalid_feature_combination) +<<

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm updated this revision to Diff 485110. arsenm added a comment. Fix unknown target handling, diagnose some more of the errors CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 Files: clang/include/clang/Basic/BuiltinsAMDGPU.def

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm planned changes to this revision. arsenm added a comment. This doesn't work correctly for unspecified wavesize for non-wave32 targets CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 ___

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added inline comments. Comment at: clang/test/CodeGenOpenCL/amdgpu-features.cl:7 +// RUN: %clang_cc1 -triple amdgcn -S -emit-llvm -o - %s | FileCheck --check-prefix=NOCPU %s +// RUN: %clang_cc1 -triple amdgcn -target-feature +wavefrontsize32 -S -emit-llvm -o - %s |

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

sameerds added inline comments. Comment at: clang/test/CodeGenOpenCL/amdgpu-features.cl:7 +// RUN: %clang_cc1 -triple amdgcn -S -emit-llvm -o - %s | FileCheck --check-prefix=NOCPU %s +// RUN: %clang_cc1 -triple amdgcn -target-feature +wavefrontsize32 -S -emit-llvm -o - %s |

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm updated this revision to Diff 484980. arsenm added a comment. Rebase. Use _w32/_w64 suffixes since some other wave specific builtins seem to have gone with that convention CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 Files:

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

JonChesterfield added a comment. In D82087#3803464 , @arsenm wrote: > In D82087#3797883 , @jdoerfert wrote: > >> Can we land this? I'd like to use the new intrinsics as I don't understand >> the old ones. > > What

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added a comment. In D82087#3797883 , @jdoerfert wrote: > Can we land this? I'd like to use the new intrinsics as I don't understand > the old ones. What do you think about using the two separate builtins, vs. one magic builtin that auto-changes

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

jdoerfert added a comment. Herald added a subscriber: kosarev. Herald added a project: All. Can we land this? I'd like to use the new intrinsics as I don't understand the old ones. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

JonChesterfield added a comment. A macro for wavefront size would make targeting gfx10 for openmp easier. We currently use an int32_t for nvptx and an int64_t for amdgcn in various runtime function interfaces. I'd like to be able to set the latter based on said macro. CHANGES SINCE LAST

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

yaxunl added a comment. In D82087#2146170 , @sameerds wrote: > >> https://github.com/ROCm-Developer-Tools/HIP/blob/master/docs/markdown/hip_kernel_language.md#warp-vote-and-ballot-functions > > > > I think if the language interface insists on fixing the

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

sameerds added a comment. >> https://github.com/ROCm-Developer-Tools/HIP/blob/master/docs/markdown/hip_kernel_language.md#warp-vote-and-ballot-functions > > I think if the language interface insists on fixing the wave size, then I > think the correct solution is to implement this in the header

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added a comment. In D82087#2140778 , @sameerds wrote: > The documentation for HIP __ballot seems to indicate that the user does not > have to explicitly specify the warp size. How is that achieved with these new > builtins? Can this be captured

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm marked 2 inline comments as done. arsenm added inline comments. Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:288 + if (!IsNullCPU) { +// Default to wave32 if available, or wave64 if not +if (Features.count("wavefrontsize32") == 0 && sameerds

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added a comment. In D82087#2140778 , @sameerds wrote: > The documentation for HIP __ballot seems to indicate that the user does not > have to explicitly specify the warp size. How is that achieved with these new > builtins? Can this be captured

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

yaxunl added inline comments. Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:293 +"wavefrontsize32" : "wavefrontsize64"; + Features.insert(std::make_pair(DefaultWaveSizeFeature, true)); +} what's the default wave front size in backend for gfx10*

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

sameerds added a comment. The documentation for HIP __ballot seems to indicate that the user does not have to explicitly specify the warp size. How is that achieved with these new builtins? Can this be captured in a lit test here?

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82087/new/ https://reviews.llvm.org/D82087 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

arsenm created this revision. arsenm added reviewers: yaxunl, rampitec, b-sumner, foad, nhaehnle. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, wdng, jvesely, kzhuravl. I wasn't sure what the best strategy was for the wave size difference. I went for an explicit, enforced builtin for