[SYCL] Optimize NDRDescT by removing sycl::range, sycl::id and padding

[DBDuncan]

sycl::range and sycl::id perform validity checks every time setting them. Use std::array instead as dimensions should already be valid. In addition, remove explicitly padding dimensions smaller than 3 and get number of dimensions from template argument instead of function argument.

[DBDuncan]

Working on improving performance with this PR has lead me to hopefully make it more explicit in what values the members of NDRDescT are set to when constructed. There are a lot of tests that rely on 1 or 0 to be set for dimensions that are not used and other behaviour I have had to preserve to get the CI to pass.

Not looking to fix the root cause of that here but to at least hopefully make it more obvious what is going on.

[DBDuncan]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

[aelovikov-intel]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

Change them to asserts maybe, if the spec doesn't require them. Or move up the callstack if the spec says the users of these classes must throw.

[DBDuncan]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

Change them to asserts maybe, if the spec doesn't require them. Or move up the callstack if the spec says the users of these classes must throw.

Looks to be from this extension: sycl/doc/extensions/proposed/sycl_ext_codeplay_cuda_cluster_group.asciidoc

I do not see any requirement to throw so asserts should be fine.

[DBDuncan]

@aelovikov-intel @Pennycook friendly ping. Double checking Aelovikov, you would like me to remove the throw in getExternalSemaphore() in sycl/source/detail/cg.hpp? Otherwise feedback is appreciated!

[DBDuncan]

@aelovikov-intel Friendly ping

[DBDuncan]

Ah sorry, I did not notice you are on vacation. I did not see until hovering over your username and missed it on your profile page.

[aelovikov-intel]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

Change them to asserts maybe, if the spec doesn't require them. Or move up the callstack if the spec says the users of these classes must throw.

Looks to be from this extension: sycl/doc/extensions/proposed/sycl_ext_codeplay_cuda_cluster_group.asciidoc

I do not see any requirement to throw so asserts should be fine.

I'm not sure how to understand this reply. The PR seems to be using std::array still and not a "lighter" "noexcept" sycl::range.

[DBDuncan]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

Change them to asserts maybe, if the spec doesn't require them. Or move up the callstack if the spec says the users of these classes must throw.

Looks to be from this extension: sycl/doc/extensions/proposed/sycl_ext_codeplay_cuda_cluster_group.asciidoc I do not see any requirement to throw so asserts should be fine.

I'm not sure how to understand this reply. The PR seems to be using std::array still and not a "lighter" "noexcept" sycl::range.

Ah, I realise now that I misunderstood you.

Your original post was asking about removing the throw from sycl;:range/sycl::id themselves and replacing the operator[] dimension check with an assert.

I am currently asking a co-worker about this idea. Looking at the sycl spec it does not seem to say anything about if these classes should throw. I am a bit uncertain as my immediate gut thought is that anything that a user could conceivably do easily should throw and asserts should be used more for sanity checks.

[DBDuncan]

Can we remove the throw from these SYCL classes instead?

Sorry, to clarify, do you mean to remove the throws as well @aelovikov-intel ? Or remove the throw's instead of something else?

Change them to asserts maybe, if the spec doesn't require them. Or move up the callstack if the spec says the users of these classes must throw.

Looks to be from this extension: sycl/doc/extensions/proposed/sycl_ext_codeplay_cuda_cluster_group.asciidoc I do not see any requirement to throw so asserts should be fine.

I'm not sure how to understand this reply. The PR seems to be using std::array still and not a "lighter" "noexcept" sycl::range.

Ah, I realise now that I misunderstood you.

Your original post was asking about removing the throw from sycl;:range/sycl::id themselves and replacing the operator[] dimension check with an assert.

I am currently asking a co-worker about this idea. Looking at the sycl spec it does not seem to say anything about if these classes should throw. I am a bit uncertain as my immediate gut thought is that anything that a user could conceivably do easily should throw and asserts should be used more for sanity checks.

I have gotten an answer from Gordon Brown. So turns out the spec deliberately does not mandate any kind of error checking for things like range/id subscript operator and leaves it up to the implementation. So it is OK to assert rather than throw.

I am unsure though as it is always good to catch errors that the use could make easily. I do think this PR removes the main performance issue of having throws in sycl::range and sycl::id as before it was performing a bunch of subscript operations on them at once. It definitely does increase performance but I have not compared against removing the throw with assert.

I think there is also a separate argument that sycl specific objects such as sycl::range/sycl::id should not be used so far into the runtime and instead std containers should be used instead if there is not a good reason. This far into the runtime sycl::range/sycl::id are basically being used as std::arrays with extra restrictions/baggage and less tested.

[ProGTX]

I don't think we should be replacing throws with asserts in id and range classes in this PR, that seems out of scope.

Also friendly ping @intel/llvm-reviewers-runtime for reviews.

[DBDuncan]

Thanks @aelovikov-intel. I have replied to your comment above. Just want to make sure you have the chance to read it before this PR gets merged. Otherwise I plan to get this PR merged pretty soon.

[steffenlarsen]

@DBDuncan - This PR was an ABI break as it removed symbols. We need those symbols back ASAP or we will have to revert this patch.