Bugfix to decay Policy for `__result_and_scratch_storage` #2031

danhoeflinger · 2025-01-30T22:19:58Z

Always decay the policy before use in __result_and_scratch_storage.

This also reverts some of #1997 which was working around this bug in the usage of __result_and_scratch_storage.
Also, there are some minor improvements to the template arguments of the reduce submitter to require the decayed policy used in __result_and_scratch_storage to match the policy used to submit.

Signed-off-by: Dan Hoeflinger <[email protected]>

(reverting part of #1997) Signed-off-by: Dan Hoeflinger <[email protected]>

Signed-off-by: Dan Hoeflinger <[email protected]>

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h

SergeyKopienko · 2025-01-31T11:19:04Z

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h

@@ -1369,8 +1371,8 @@ __parallel_partition_copy(oneapi::dpl::__internal::__device_backend_tag __backen
            oneapi::dpl::__par_backend_hetero::__write_to_id_if_else<oneapi::dpl::__internal::__pstl_assign>;
        try
        {
-            return __parallel_reduce_then_scan_copy(__backend_tag, __exec, __rng, __result, __n, _GenMask{__pred},
-                                                    _WriteOp{},
+            return __parallel_reduce_then_scan_copy(__backend_tag, std::forward<_ExecutionPolicy>(__exec), __rng,


SergeyKopienko · 2025-01-31T11:19:24Z

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h

@@ -1543,17 +1546,18 @@ __parallel_set_op(oneapi::dpl::__internal::__device_backend_tag __backend_tag, c
    {
        try
        {
-            return __parallel_set_reduce_then_scan(__backend_tag, __exec, __rng1, __rng2, __result, __comp,
-                                                   __is_op_difference);
+            return __parallel_set_reduce_then_scan(__backend_tag, std::forward<_ExecutionPolicy>(__exec), __rng1,


SergeyKopienko · 2025-01-31T11:19:42Z

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h

@@ -2492,8 +2497,8 @@ __parallel_reduce_by_segment(oneapi::dpl::__internal::__device_backend_tag, cons
            try
            {
                auto __res = oneapi::dpl::__par_backend_hetero::__parallel_reduce_by_segment_reduce_then_scan(
-                    oneapi::dpl::__internal::__device_backend_tag{}, __exec, __keys, __values, __out_keys, __out_values,
-                    __binary_pred, __binary_op);
+                    oneapi::dpl::__internal::__device_backend_tag{}, std::forward<_ExecutionPolicy>(__exec), __keys,


SergeyKopienko · 2025-01-31T11:23:06Z

@danhoeflinger , you wrote in description of this PR:
"This also reverts some of #1997 which was working around this bug in the usage of __result_and_scratch_storage."

May you explain how this bug has been fixed on __result_and_scratch_storage level?
Looks like it hasn't been fixed...

danhoeflinger · 2025-01-31T13:46:08Z

@danhoeflinger , you wrote in description of this PR: "This also reverts some of #1997 which was working around this bug in the usage of __result_and_scratch_storage."

May you explain how this bug has been fixed on __result_and_scratch_storage level? Looks like it hasn't been fixed...

The bug is in the usage of __result_and_scratch_storage, not the type itself. That part is fixed with this PR, because we decay the policy type everywhere we are specifying the type of __result_and_scratch_storage (not during overloads where we are instead deducing the type). We can still think about if there is a better way to "get" the type of __result_and_scratch_storage, without needing to always think about decaying of a policy.

Perhaps we can add a using statement to define __result_and_scratch_storage_t which automatically decays the policy type...

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger · 2025-01-31T14:19:48Z

@mmichel11 @SergeyKopienko please look at #2033, do we like that additional change?

It makes usage easier, but obfuscates what is going on.

Edit: I've incorporated those changes. I think they are for the better on the whole.

* improve usage pattern for reduce_and_scratch_storage Signed-off-by: Dan Hoeflinger <[email protected]> * adjust names Signed-off-by: Dan Hoeflinger <[email protected]> * missed a couple names to change Signed-off-by: Dan Hoeflinger <[email protected]> * adding comment Signed-off-by: Dan Hoeflinger <[email protected]> * Formatting Signed-off-by: Dan Hoeflinger <[email protected]> --------- Signed-off-by: Dan Hoeflinger <[email protected]>

mmichel11

LGTM

always decay execution policy for result_and_scratch_storage

a2f7eb7

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger requested review from SergeyKopienko and mmichel11 January 30, 2025 22:19

danhoeflinger added this to the 2022.8.0 milestone Jan 30, 2025

formatting

3b7f0e0

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger mentioned this pull request Jan 30, 2025

Work around crashes and incorrect results in scan-based algorithms when compiling with -O0 #1997

Merged

moving back to forwarding references

de56024

(reverting part of #1997) Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger marked this pull request as draft January 31, 2025 00:56

danhoeflinger added 2 commits January 30, 2025 19:58

remove decay in future overload

768f45f

Signed-off-by: Dan Hoeflinger <[email protected]>

fixing reduce policy decaying

c2215df

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger marked this pull request as ready for review January 31, 2025 02:23

mmichel11 reviewed Jan 31, 2025

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Jan 31, 2025

View reviewed changes

revert forwarding policy within try block

a60309b

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger requested a review from akukanov January 31, 2025 16:14

mmichel11 approved these changes Jan 31, 2025

View reviewed changes

danhoeflinger merged commit 7bbaf83 into main Jan 31, 2025
22 checks passed

danhoeflinger deleted the dev/dhoeflin/decay_policy_result_scratch branch January 31, 2025 19:11

mmichel11 mentioned this pull request Feb 3, 2025

Fix duplicate kernel naming in reduce-then-scan kernels #2040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix to decay Policy for `__result_and_scratch_storage` #2031

Bugfix to decay Policy for `__result_and_scratch_storage` #2031

danhoeflinger commented Jan 30, 2025 •

edited

Loading

SergeyKopienko Jan 31, 2025

danhoeflinger Jan 31, 2025

SergeyKopienko Jan 31, 2025

danhoeflinger Jan 31, 2025

SergeyKopienko Jan 31, 2025

danhoeflinger Jan 31, 2025

SergeyKopienko commented Jan 31, 2025 •

edited

Loading

danhoeflinger commented Jan 31, 2025

danhoeflinger commented Jan 31, 2025 •

edited

Loading

mmichel11 left a comment

Bugfix to decay Policy for __result_and_scratch_storage #2031

Bugfix to decay Policy for __result_and_scratch_storage #2031

Conversation

danhoeflinger commented Jan 30, 2025 • edited Loading

SergeyKopienko Jan 31, 2025

Choose a reason for hiding this comment

danhoeflinger Jan 31, 2025

Choose a reason for hiding this comment

SergeyKopienko Jan 31, 2025

Choose a reason for hiding this comment

danhoeflinger Jan 31, 2025

Choose a reason for hiding this comment

SergeyKopienko Jan 31, 2025

Choose a reason for hiding this comment

danhoeflinger Jan 31, 2025

Choose a reason for hiding this comment

SergeyKopienko commented Jan 31, 2025 • edited Loading

danhoeflinger commented Jan 31, 2025

danhoeflinger commented Jan 31, 2025 • edited Loading

mmichel11 left a comment

Choose a reason for hiding this comment

Bugfix to decay Policy for `__result_and_scratch_storage` #2031

Bugfix to decay Policy for `__result_and_scratch_storage` #2031

danhoeflinger commented Jan 30, 2025 •

edited

Loading

SergeyKopienko commented Jan 31, 2025 •

edited

Loading

danhoeflinger commented Jan 31, 2025 •

edited

Loading