[SYCL][Doc] Update if_architecture_is extension to include NVIDIA and AMD architectures #7246

mmoadeli · 2022-11-01T12:08:24Z

Update if_architecture_is extension to include NVIDIA and AMD architectures

For NVIDIA adds aspect for each sm version,
For AMD adds aspect for each architecture supported by ROCm,
Copies updated version of experimental/sycl_ext_intel_device_architecture.asciidoc to proposed/sycl_ext_oneapi_device_architecture.asciidoc.

…architectures - For NVIDI adds aspect for each sm version, - For AMD adds aspect for each architecture supported by ROCm.

bader · 2022-11-01T12:19:14Z

Don't forget to rename the file too.

sycl/doc/extensions/experimental/sycl_ext_intel_device_architecture.asciidoc

…_oneapi_device_architecture.asciidoc. - Minor update to reflect recent changes on cuda architecure additions.

mmoadeli · 2022-11-01T14:39:09Z

Don't forget to rename the file too.

Thanks @bader, done.

Pennycook · 2022-11-01T15:12:58Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc

+|`nvidia_gpu_sm30`
+|1
+|NVIDIA Kepler architecture.
+
+|`nvidia_gpu_sm32`
+|1
+|NVIDIA Kepler architecture.


Listing the same architecture here is confusing, and I suspect some readers will struggle to understand the difference between the sm30, sm32, sm35 and sm37 lines in the table.

Why not list the compute capability too, to make it obvious that's what the number means? e.g.

nvidia_gpu_sm30, NVIDIA Kepler architecture (compute capability 3.0)

nvidia_gpu_sm32, NVIDIA Kepler architecture (compute capability 3.2)

I think it might also be a good idea to add a non-normative note above or below the table that says something like:

"For NVIDIA GPUs, the architecture enumerator corresponds to the compute capability of the device, and ext_oneapi_architecture_is can be used similarly to the __CUDA_ARCH__ macro in CUDA."

...since it may help folks migrating from CUDA to SYCL.

thanks @Pennycook for review. I applied your comments in.

gmlueck · 2022-11-01T17:09:13Z

Thanks for adding this!

Could you also update the design document to describe how this will be implemented? I presume we will add new target names for the -fsycl-targets compiler option? If so, it is sufficient to update that design document to add the new command line options to the list and to add the matching predefined macro names to the list. If the implementation is somehow more complicated, we should discuss.

Assuming this is not going to be implemented in the same PR, we need to separate the part that is currently implemented from the part that is not yet implemented. Since this extension specification is in the "experimental" directory, it is currently implemented, and customers can rely on this specification to know how to use this feature. Simply adding to the specification breaks that contract because customers will no longer know what is vs. is not implemented. We usually solve this by creating a copy of the specification in the "proposed" directory and making that changes there. Once it is implemented, we move the proposed document to the "experimental" directory, overwriting the previous version. This way, the document in "experimental" always describes what is currently implemented.

gmlueck · 2022-11-01T17:10:40Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc

@@ -98,7 +98,7 @@ implementation supports.
 This extension adds a new enumeration of the architectures that can be tested.


This comment is really for the table above, which you did not change. Please add a new row to that table for version 2 of this specification, noting that the Nvidia and AMD architectures were added in version 2.

thanks @gmlueck. done.

gmlueck · 2022-11-01T17:11:48Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc

@@ -295,12 +337,176 @@ of these enumerators, and it provides a brief description of their meanings.
 |`intel_gpu_12_10_0`
 |1
 |Alias for `intel_gpu_dg1`.
+
+|`nvidia_gpu_sm20`
+|1


In addition, change all these new entries to 2, so that users know that these entries were added in version 2 of the specification.

thanks @gmlueck. done.

- Reflects the version addition to the document.

…oc to proposed folder. - Reflect updates to sycl_ext_oneapi_device_architecture.asciidoc into DeviceIf.md - Reverts changes made to experimental/sycl_ext_intel_device_architecture.asciidoc to avoid confustion on what is and what is not yet implemented.

mmoadeli · 2022-11-02T15:42:04Z

Thanks for adding this!

Could you also update the design document to describe how this will be implemented? I presume we will add new target names for the -fsycl-targets compiler option? If so, it is sufficient to update that design document to add the new command line options to the list and to add the matching predefined macro names to the list. If the implementation is somehow more complicated, we should discuss.

Assuming this is not going to be implemented in the same PR, we need to separate the part that is currently implemented from the part that is not yet implemented. Since this extension specification is in the "experimental" directory, it is currently implemented, and customers can rely on this specification to know how to use this feature. Simply adding to the specification breaks that contract because customers will no longer know what is vs. is not implemented. We usually solve this by creating a copy of the specification in the "proposed" directory and making that changes there. Once it is implemented, we move the proposed document to the "experimental" directory, overwriting the previous version. This way, the document in "experimental" always describes what is currently implemented.

@gmlueck

The updates are reflected into design document
The updated version of experimental/sycl_ext_intel_device_architecture.asciidoc is copied into proposed/sycl_ext_oneapi_device_architecture.asciidoc.
experimental/sycl_ext_intel_device_architecture.asciidoc is reverted to it's original state.

gmlueck

This looks good. Just a couple of small comments below.

gmlueck · 2022-11-02T21:09:39Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_device_architecture.asciidoc

+  intel_gpu_12_0_0 = intel_gpu_tgllp,
+  intel_gpu_12_10_0 = intel_gpu_dg1,
+
+  nvidia_gpu_sm20,


All these "nvidia" and "amd" enumerators should go before the "alias" enumerators like intel_gpu_8_0_0. Otherwise, the "nvidia" and "amd" enumerators will alias some of the "intel" ones.

that's right @gmlueck, thanks . done.

gmlueck · 2022-11-02T21:14:43Z

sycl/doc/design/DeviceIf.md

@@ -249,7 +329,7 @@ constexpr static auto if_architecture_is(T fnTrue, Args ...args) {
  }
 }

-} // namespace ext::intel::experimental
+} // namespace ext::oneapi::exprimental
 } // namespace sycl
 ```



This comment is for the sentence below that says:

The only supported targets are spir64_x86_64 and the new intel_gpu_* GPU device names.

I think that sentence should be updated to include the "nvidia" and "amd" device names.

thanks @gmlueck, done.

- Adds amd and nvidia gpus as supported targets.

[SYCL] Update if_architecture_is extension to include Nvidia and AMD …

faca015

…architectures - For NVIDI adds aspect for each sm version, - For AMD adds aspect for each architecture supported by ROCm.

mmoadeli requested a review from a team as a code owner November 1, 2022 12:08

bader reviewed Nov 1, 2022

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_intel_device_architecture.asciidoc Outdated Show resolved Hide resolved

[SYCL] Rename sycl_ext_intel_device_architecture.asciidoc to sycl_ext…

d4e0b03

…_oneapi_device_architecture.asciidoc. - Minor update to reflect recent changes on cuda architecure additions.

Pennycook reviewed Nov 1, 2022

View reviewed changes

[SYCL][CUDA] Clarifies NVIDIA GPU architectures enumarators.

d9791ba

gmlueck reviewed Nov 1, 2022

View reviewed changes

mmoadeli changed the title ~~[SYCL] Update if_architecture_is extension to include NVIDIA and AMD architectures~~ [SYCL][Doc] Update if_architecture_is extension to include NVIDIA and AMD architectures Nov 2, 2022

mmoadeli added 2 commits November 2, 2022 09:39

[SYCL][Doc] Adds version for nvidia and amd architecture.

b72788c

- Reflects the version addition to the document.

gmlueck reviewed Nov 2, 2022

View reviewed changes

mmoadeli added 2 commits November 2, 2022 23:44

[SYCL][Doc] Moves intel gpu aliases to the end of enum class.

146b39c

- Adds amd and nvidia gpus as supported targets.

[SYCL][Doc] Minor style update.

ea2a108

gmlueck approved these changes Nov 3, 2022

View reviewed changes

pvchupin merged commit c6091df into intel:sycl Nov 3, 2022

mmoadeli deleted the amd-gpu-ext-arch branch July 7, 2023 10:43

		@@ -98,7 +98,7 @@ implementation supports.
		This extension adds a new enumeration of the architectures that can be tested.

[SYCL][Doc] Update if_architecture_is extension to include NVIDIA and AMD architectures #7246

[SYCL][Doc] Update if_architecture_is extension to include NVIDIA and AMD architectures #7246

Uh oh!

Conversation

mmoadeli commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Nov 1, 2022

Uh oh!

Uh oh!

mmoadeli commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlueck commented Nov 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmoadeli commented Nov 2, 2022

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmoadeli commented Nov 1, 2022 •

edited

Loading

mmoadeli commented Nov 1, 2022 •

edited

Loading