Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][CUDA][HIP] Implement support for AMD and NVIDIA architectures as argument to fsycl-targets #7348

Merged
merged 19 commits into from
Dec 10, 2022

Conversation

mmoadeli
Copy link
Contributor

@mmoadeli mmoadeli commented Nov 10, 2022

  • Update if_architecture_is extension to include NVIDIA and AMD architectures.
  • Moves if_architecture_is code header file from intel to oneapi.
    *Updates experimental/sycl_ext_intel_device_architecture.asciidoc with the contents from proposed/sycl_ext_oneapi_device_architecture.asciidoc.
  • Rename sycl_ext_intel_device_architecture.asciidoc to sycl_ext_oneapi_device_architecture.asciidoc.
  • Delete proposed/ycl_ext_oneapi_device_architecture.asciidoc.
  • Renames nvidia_gpu_smxx to nvidia_gpu_sm_xx.
  • Remove DPC++ un-supported architectures for nvidia (sm_20 to sm_37)

…IA and AMD architectures

- Reflect earlier updates in DeviceIf.md to the implementation.
- Reflect above update into the related unit test.
…sciidoc with the contents from proposed/sycl_ext_oneapi_device_architecture.asciidoc
…l_ext_oneapi_device_architecture.asciidoc.

- Delete proposed/ycl_ext_oneapi_device_architecture.asciidoc
@mmoadeli mmoadeli requested review from a team as code owners November 10, 2022 09:30
@gmlueck
Copy link
Contributor

gmlueck commented Nov 10, 2022

Don't we also need changes to the compiler driver to predefine macros like __SYCL_TARGET_NVIDIA_GPU_SM20__?

@dm-vodopyanov
Copy link
Contributor

@mmoadeli thanks for the PR! Please don't forget to update tests in intel/llvm-test-suite: https://github.com/intel/llvm-test-suite/blob/intel/SYCL/DeviceArchitecture/device_architecture.cpp

@mmoadeli
Copy link
Contributor Author

@mmoadeli thanks for the PR! Please don't forget to update tests in intel/llvm-test-suite: https://github.com/intel/llvm-test-suite/blob/intel/SYCL/DeviceArchitecture/device_architecture.cpp

thanks @dm-vodopyanov , i'll update the tests.

@mmoadeli
Copy link
Contributor Author

Don't we also need changes to the compiler driver to predefine macros like __SYCL_TARGET_NVIDIA_GPU_SM20__?

thanks @gmlueck , I address this.

- Fixes an isssue in getGenDeviceMacro returning StringRef to locally created string.
- Updates user mantual to add supported nvidia and amd gpus.
@mmoadeli mmoadeli requested review from a team as code owners November 14, 2022 17:08
clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved
clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved
sycl/doc/UsersManual.md Outdated Show resolved Hide resolved
@mmoadeli
Copy link
Contributor Author

@mmoadeli thanks for the PR! Please don't forget to update tests in intel/llvm-test-suite: https://github.com/intel/llvm-test-suite/blob/intel/SYCL/DeviceArchitecture/device_architecture.cpp
@dm-vodopyanov done 1402

@mmoadeli
Copy link
Contributor Author

Don't we also need changes to the compiler driver to predefine macros like __SYCL_TARGET_NVIDIA_GPU_SM20__?

@gmlueck, thanks for your comment. It's addressed.

@dm-vodopyanov
Copy link
Contributor

/verify with intel/llvm-test-suite#1402

Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc and spec changes LGTM, but you should still get approvals for the driver and header changes.

@mmoadeli
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1402

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Nov 24, 2022

@v-klochkov @mdtoguchi @dm-vodopyanov
would anyone please review this PR? One approval needed to have it merged.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sycl/doc/UsersManual.md changes look good to me.

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 8, 2022

/verify with intel/llvm-test-suite#1402

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 9, 2022

@AlexeySachkov any chance to have this PR merged please?

@AlexeySachkov
Copy link
Contributor

@mmoadeli, from what I see, this PR still lacks an approval from @intel/llvm-reviewers-runtime and I don't have enough permissions to merge without approvals.

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 9, 2022

@mmoadeli, from what I see, this PR still lacks an approval from @intel/llvm-reviewers-runtime and I don't have enough permissions to merge without approvals.
@AlexeySachkov, thanks to @dm-vodopyanov, we have approval on behalf of @intel/llvm-reviewers-runtime

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 9, 2022

@v-klochkov would you mind reviewing this PR please?

@pvchupin
Copy link
Contributor

pvchupin commented Dec 9, 2022

@mmoadeli, there is a fail on opencl for modified test and some documentation issue. Can you take a look?

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 9, 2022

@mmoadeli, there is a fail on opencl for modified test and some documentation issue. Can you take a look?

Thanks @pvchupin
The failure on opencl relates to the issue which is fixed in this PR. Running the llvm-test-suite yesterday with this PR did not show the failure.

Would you please clarify on the issues with documentations?

@pvchupin
Copy link
Contributor

pvchupin commented Dec 10, 2022

Would you please clarify on the issues with documentations?

Generate Doxygen CI task fails. It seems it was fixed in #7524, but
CI picks up older base

@pvchupin pvchupin merged commit e5de913 into intel:sycl Dec 10, 2022
@pvchupin
Copy link
Contributor

@mmoadeli, can you take a look into post-commit issue on Windows please?
https://github.com/intel/llvm/actions/runs/3662283607/jobs/6191278254

 Clang :: Driver/sycl-oneapi-gpu.cpp

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Dec 12, 2022

@mmoadeli, can you take a look into post-commit issue on Windows please? https://github.com/intel/llvm/actions/runs/3662283607/jobs/6191278254

 Clang :: Driver/sycl-oneapi-gpu.cpp

@pvchupin, sorry, seems some tests are assumed linux build. I address them shortly.

@mmoadeli mmoadeli deleted the nvida-amd-imp-ext branch July 7, 2023 10:43
acmnpv pushed a commit to gromacs/gromacs that referenced this pull request Nov 28, 2023
A shorter syntax to specify device targets has been introduced in
intel/llvm#7348:

* `-DSYCL_CXX_FLAGS_EXTRA='-fsycl-targets=nvidia_gpu_sm_86'`, or
* `-DSYCL_CXX_FLAGS_EXTRA='-fsycl-targets=amd_gpu_gfx906'`.

Now, our CMake correctly parses these flags too.

Refs #4716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants