[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. Thanks, @yaxunl. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa6786cdd5757: [HIPSPV][3/4] Enable SPIR-V emission for HIP (authored by yaxunl). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added a comment. In D110622#3199233 , @linjamaki wrote: > Assuming that this patch is ready to land. @tra or @yaxunl, could you please > commit this patch to the LLVM for us? Thanks. I can help commit this patch. Repository: rG LLVM Github

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. Assuming that this patch is ready to land. @tra or @yaxunl, could you please commit this patch to the LLVM for us? Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 394789. linjamaki added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 Files: clang/include/clang/Basic/Cuda.h

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. Thanks for the feedback. The `--offload` is meant to support multiple targets but right now it is restricted to one at most. The limitation comes from the HIPActionBuilder and CudaActionBuilderBase which currently expects a single target triple and toolchain for all

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 392675. linjamaki added a comment. - Add comments to clarify the limitation of the `--offload` option to one target. - Add test for multiple `--offload` option instances. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added inline comments. Comment at: clang/lib/Driver/Driver.cpp:105-109 +auto HIPOffloadTargets = Args.getAllArgValues(options::OPT_offload_EQ); +switch (HIPOffloadTargets.size()) { +default: + D.Diag(diag::err_drv_only_one_offload_target_supported_in) <<

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

tra added a comment. In D110622#3177490 , @yaxunl wrote: > I don't think `--offload=` is restricted to be specified only once. The test > checks `--offload-arch=` and `--offload=` are mutually exclusive. It effectively is. See my inline comment. //

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added a comment. In D110622#3177111 , @tra wrote: > In D110622#3176804 , @yaxunl wrote: > So, the question is -- what's the right way to specify something like this in a consistent manner?

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

tra added a comment. In D110622#3176804 , @yaxunl wrote: >>> So, the question is -- what's the right way to specify something like this >>> in a consistent manner? >>> `--offload` option proposed here does not seem to be a good fit. It was >>>

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added a comment. >> So, the question is -- what's the right way to specify something like this >> in a consistent manner? >> `--offload` option proposed here does not seem to be a good fit. It was >> intended as a more flexible way to create a single `-cc1` sub-compilation >> and we're

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

bader added a comment. In D110622#3174113 , @tra wrote: > The patch looks OK for the time being. That said, I do have concerns that we > may be organically growing something that will be troublesome to deal with > long-term. > > TBH, I still can't

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

tra accepted this revision. tra added a comment. This revision is now accepted and ready to land. Note to self: don't forget to hit "submit". The comments below have been left unsubmitted for two weeks. Sorry about that. The patch looks OK for the time being. That said, I do have concerns that

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. @tra, gentle ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 389114. linjamaki marked 2 inline comments as done. linjamaki added a comment. Update a driver test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 Files:

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added a comment. LGTM. I will defer to @tra Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki marked 2 inline comments as done. linjamaki added inline comments. Comment at: clang/include/clang/Basic/Cuda.h:109 + // Generic processor model is for testing only. + return A >= CudaArch::GFX600 && A <= CudaArch::GFX1035; } yaxunl wrote: > can we

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 388146. linjamaki added a comment. - Adjust `--offload` description: reflect the current state. - Adjust enum range check in IsAMDGpuArch(). - Make `--offload` and `--offload-arch` options mutually exclusive. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

yaxunl added inline comments. Comment at: clang/include/clang/Basic/Cuda.h:109 + // Generic processor model is for testing only. + return A >= CudaArch::GFX600 && A <= CudaArch::GFX1035; } can we use A < CudaArch::Generic instead? to avoid updating this line

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. Gentle ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 387509. linjamaki added a comment. Herald added a subscriber: asavonic. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 Files:

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 382244. linjamaki added a comment. Improve `--offload` option description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 Files:

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 381855. linjamaki added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 Files: clang/include/clang/Basic/Cuda.h

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki updated this revision to Diff 376829. linjamaki added a comment. Repurpose 'Generic' CudaArch, Use getAllArgValues() for reading --offload values and fix a enum range. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki added a comment. In D110622#3030792 , @tra wrote: >> A Cuda GPU architecture ‘generic’ is added. The name is picked from the LLVM >> SPIR-V Backend. In the HIPSPV code path the architecture name is inserted to >> the bundle entry ID as target

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

tra added a comment. > --offload’ option, which is envisioned in [1], is added for specifying > offload targets. This option is used to override default device target > (amdgcn-amd-amdhsa) for HIP compilation for emitting device code as SPIR-V > binary. The option is handled in

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

tra added a comment. > A Cuda GPU architecture ‘generic’ is added. The name is picked from the LLVM > SPIR-V Backend. In the HIPSPV code path the architecture name is inserted to > the bundle entry ID as target ID. Target ID is expected to be always present > so a component in the target

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

linjamaki created this revision. Herald added subscribers: Naghasan, dexonsmith, dang, Anastasia, yaxunl, jholewinski. linjamaki updated this revision to Diff 375783. linjamaki added a comment. linjamaki updated this revision to Diff 375786. linjamaki added reviewers: yaxunl, bader. linjamaki