[SYCL] Fix the type trait 'known_identity' #3227

v-klochkov · 2021-02-18T03:30:03Z

Copy 'known_identity' and 'has_known_identity' from sycl::ONEAPI to sycl
Fix the compilation errors reported on attempts to use vector types
like sycl::int4 as a reduction variable.

Signed-off-by: Vyacheslav N Klochkov [email protected]

- Move 'known_identity' and 'has_known_identity' from sycl::ONEAPI to sycl - Fix the compilation errors reported on attempts to use vector types like sycl::int4 as a reduction variable. Signed-off-by: Vyacheslav N Klochkov <[email protected]>

v-klochkov · 2021-02-18T03:41:49Z

This patch only fixes a correctness problem caused by using vec<int,N> types.
Potentially, known_identity< std::plus<>, int4 > could return a constexpr value, but that would require additional elaborate changes in vec class, and that is a lower priority task than the stability crash fixed in this pull request.

Pennycook

I'm okay with this change as a temporary fix, but we need to revisit this issue. It's not clear to me whether has_known_identity requiring constexpr is too restrictive, or whether types like vec should be made constexpr.

The SYCL 2020 specification doesn't seem to say whether an implementation is expected to be able to determine the identity value for vector types, and it probably comes down to whether or not a vector counts as "an arithmetic type" (see https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:reduction). This probably requires some clarification.

Pennycook · 2021-02-18T16:41:10Z

sycl/include/CL/sycl/ONEAPI/reduction.hpp

 template <typename BinaryOperation, typename AccumulatorT>
-struct has_known_identity : detail::has_known_identity_impl<
+struct has_known_identity : ONEAPI::detail::has_known_identity_impl<


I don't know the answer to this, but I want to make sure that we're being consistent across different extensions and SYCL 2020 features. Should this be available as ONEAPI::has_known_identity in SYCL 1.2.1 mode, and only available as has_known_identity in SYCL 2020 mode?

only available as has_known_identity in SYCL 2020 mode

John, I tried implementing what you suggested in 9b355ef and 389fe9b
When you say "only in SYCL 2020 mode" what do you mean?
I don't see good ways to enable it only for SYCL-2020. In my opinion, sycl::has_known_identity should be available by default in headers as SYCL-2020 seems to be the default now with -fsycl. Guarding sycl::has_known_identity with some macros like SYCL_LANGUAGE_VERSION >= 202001L seems not very usable as it would break compilation of SYCL-2020 programs without using -fsycl switch.

I was assuming that we would still allow developers to request SYCL 1.2.1 -- if that isn't the case, then I'm less worried about conditionally enabling SYCL 2020 features.

That said, I think standard C++ headers typically guard features based on the standard that included them. I don't feel strongly about this -- @rolandschulz, do you think it matters at all?

If we don't (partially) support multiple SYCL versions, I don't see a reason to have features guarded.

rolandschulz · 2021-02-18T16:50:53Z

I'm okay with this change as a temporary fix, but we need to revisit this issue. It's not clear to me whether has_known_identity requiring constexpr is too restrictive, or whether types like vec should be made constexpr.

It might make sense to only support marray instead of vec and make sure that marray is constexpr.

keryell · 2021-02-18T23:55:43Z

Now SYCL 2020 is out we have more time to constexpr-ify the specification.

But I have the feeling that adding some constexpr today in an implementation should not change the compliance test and would make the implementation better, exposing more optimization opportunities.

I was about creating an issue about it but discovered I already created one... https://gitlab.khronos.org/sycl/Specification/-/issues/246 Ahem... :-/

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

…reduction_int4

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

smaslov-intel · 2021-02-26T17:46:31Z

Move 'known_identity' and 'has_known_identity' from sycl::ONEAPI to sycl

Isn't this is a backward compatibility breaking change that we aren't allowed to do except in major releases?

v-klochkov · 2021-02-26T17:51:50Z

Move 'known_identity' and 'has_known_identity' from sycl::ONEAPI to sycl

Isn't this is a backward compatibility breaking change that we aren't allowed to do except in major releases?

The later commit 9b355ef in this PR puts it back to sycl::ONEAPI. I replaced the word 'move' with the word 'copy' in the description. Thank you!

smaslov-intel

LGTM, just minor ask for a TODO comment to remember to remove the ONEAPI copy.

smaslov-intel · 2021-02-26T18:05:16Z

sycl/include/CL/sycl/ONEAPI/reduction.hpp

@@ -1659,7 +1642,30 @@ template <typename BinaryOperation, typename AccumulatorT>
 inline constexpr AccumulatorT known_identity_v =
    known_identity<BinaryOperation, AccumulatorT>::value;
 #endif
-


I think it is worth to mark the code that we want to remove when allowed to break API.

v-klochkov · 2021-02-26T18:23:32Z

for a TODO comment to remember to remove the ONEAPI

ONEAPI::has_known_identity should live as long as ONEAPI::reduction() lives.

sycl::has_known_identity will migrate from the file CL/sycl/ONEAPI/reduction.hpp to CL/sycl/reduction.hpp as soon as SYCL-2020 reductions works start, and it will be tied to sycl::reduction.

…tel#3227) The check added in (intel#3227) was supposed to return true for int32/int64, but wrongly checked the type sizes in bits instead of bytes, which prohibited usage of group ADD algorithm for int32/int64 in reductions. Fixed in this patch. Signed-off-by: Vyacheslav N Klochkov <[email protected]>

The check added in (#3227) was supposed to return true for int32/int64, but wrongly checked the type sizes in bits instead of bytes, which prohibited usage of group ADD algorithm for int32/int64 in reductions. Signed-off-by: Vyacheslav N Klochkov <[email protected]>

[SYCL] Fix the type trait 'known_identity'

701377b

- Move 'known_identity' and 'has_known_identity' from sycl::ONEAPI to sycl - Fix the compilation errors reported on attempts to use vector types like sycl::int4 as a reduction variable. Signed-off-by: Vyacheslav N Klochkov <[email protected]>

v-klochkov requested a review from Pennycook February 18, 2021 03:30

v-klochkov requested a review from a team as a code owner February 18, 2021 03:30

v-klochkov requested review from smaslov-intel and rolandschulz February 18, 2021 03:30

Pennycook reviewed Feb 18, 2021

View reviewed changes

v-klochkov added 3 commits February 22, 2021 22:36

Fix LIT test fails + address reviewer's comment

9b355ef

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

Merge remote-tracking branch 'intel_llvm/sycl' into public_vklochkov_…

0b2702b

…reduction_int4

Remove guarding macro around has_known_identity and known_identity

389fe9b

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

v-klochkov force-pushed the public_vklochkov_reduction_int4 branch from 572e590 to 389fe9b Compare February 23, 2021 21:15

v-klochkov requested a review from Pennycook February 26, 2021 15:00

Pennycook approved these changes Feb 26, 2021

View reviewed changes

smaslov-intel approved these changes Feb 26, 2021

View reviewed changes

v-klochkov merged commit 060fd50 into intel:sycl Feb 26, 2021

v-klochkov deleted the public_vklochkov_reduction_int4 branch February 26, 2021 18:23

v-klochkov mentioned this pull request May 15, 2021

[SYCL] Fix wrong checks in IsReduOptForFastReduce<> introduced in (#3… #3763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Fix the type trait 'known_identity' #3227

[SYCL] Fix the type trait 'known_identity' #3227

Uh oh!

v-klochkov commented Feb 18, 2021 •

edited

Loading

Uh oh!

v-klochkov commented Feb 18, 2021

Uh oh!

Pennycook left a comment

Uh oh!

Pennycook Feb 18, 2021

Uh oh!

v-klochkov Feb 23, 2021 •

edited

Loading

Uh oh!

Pennycook Feb 24, 2021

Uh oh!

rolandschulz Feb 24, 2021

Uh oh!

rolandschulz commented Feb 18, 2021

Uh oh!

keryell commented Feb 18, 2021

Uh oh!

smaslov-intel commented Feb 26, 2021

Uh oh!

v-klochkov commented Feb 26, 2021 •

edited

Loading

Uh oh!

smaslov-intel left a comment

Uh oh!

smaslov-intel Feb 26, 2021

Uh oh!

v-klochkov commented Feb 26, 2021

Uh oh!

Uh oh!

[SYCL] Fix the type trait 'known_identity' #3227

[SYCL] Fix the type trait 'known_identity' #3227

Uh oh!

Conversation

v-klochkov commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-klochkov commented Feb 18, 2021

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pennycook Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

rolandschulz Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

rolandschulz commented Feb 18, 2021

Uh oh!

keryell commented Feb 18, 2021

Uh oh!

smaslov-intel commented Feb 26, 2021

Uh oh!

v-klochkov commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov commented Feb 26, 2021

Uh oh!

Uh oh!

v-klochkov commented Feb 18, 2021 •

edited

Loading

v-klochkov Feb 23, 2021 •

edited

Loading

v-klochkov commented Feb 26, 2021 •

edited

Loading