Fix shift_[left/right] for small shift values by danhoeflinger · Pull Request #2824 · uxlfoundation/oneDPL

danhoeflinger · 2026-09-08T20:48:44Z

shift_[left/right] for small values is a degenerate case for the previous GPU implementation, as it describes the number of independent parallel lines of execution.
For cases where it is beneficial, this PR adds implementing of shift via rotate, which satisfies shift. For very small segment sizes, or for large enough shifts, the existing implementation is superior. Also rotate requires swappable, so we only enable this for types which support swap.

For some degenerately bad cases, this provides up to 1300x on BMG and 7000x on PVC.

__pattern_shift_left's overlapping branch passes the shift amount as the __parallel_for iteration count, so a shift of 8 launches 8 work items whatever the range length, each walking (size - n) / n dependent moves. shift_left(first, last, n) is rotate(first, first + n, last): it produces the required prefix and leaves the original head in the tail, whose content [alg.shift] does not specify. __pattern_rotate is an in-place triple reverse in two kernels, so the shift becomes fully parallel with no temporary and no capacity limit. A wide shift still fills the device on its own and the walk moves half the bytes, so __should_rotate_shift keeps the walk for a shift that is either wide or shallow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Run 31744148108 measured the rotate at n=8 and n=1,024 on the same sizes: its cost does not depend on n (within 3%), while the walk's grows as size_res/n hops. PVC admits n=1,024 already and gains 44-63x there; BMG's width/128 bound rejects it and pays 9.4-16.8x more than the rotate it could have used. The bound only has to stay clear of the n wide enough to reach full bandwidth, so width/64 - which covers the measured n=1,024 on both platforms - replaces it. The byte-denominated clause moves with it, so the two stay equal at 4-byte elements. The test's rot_size now derives from 65 * (n_max + 1), keeping the depth bound clear at n_max + 1 so the parallelism bound alone rejects it.

Address adversarial review of the rotate strategy: - Instantiate the rotate branch only for swappable element types. The reverse it runs swaps elements, which shift_left does not require of its type, so a MoveAssignable-but-not-MoveConstructible type failed to compile where it built before. - Reject n == 0 in the gate. The ranges entry point does not screen n, and with an occupancy width of 0 every bound held vacuously, so the branch was reachable on CPU and FPGA devices. - Cover the byte-denominated bound with an 8-byte type, where it rather than n <= width/64 is the deciding term, and report the occupancy width so a run that skips these cases is distinguishable from one that passes them. - Correct the gate comments: the parallelism bound is 64 lanes per chain, not 64 chains per lane, and the divisions guard a huge n rather than a 32-bit one.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

The guard added in fa6c034 referenced '__shift'; main renamed that parameter to '__n' in #2802, so the rebased branch did not compile.

'assert' compiles away under NDEBUG, and the gate divides by '__n'. Callers do screen '0 < n < size', but the sentinel width of 0 also relies on n > 0, so degrade to the walk instead of dividing by zero. Add the missing <cassert>.

The gate is now 'size_res/64 >= n && n*sizeof(T) <= width/4'; the cases still derived n_max from the retired element-count clause, so they no longer straddled the boundary they describe. Assert the GPU width instead of printing it, since ctest discards a passing test's stdout.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

Copilot

🟡 Changes recommended

The rotate path violates the shift assignment bound, and its targeted test can pass without moving data.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Optimizes small GPU shifts by using the existing rotate implementation when beneficial.

Changes:

Adds GPU/FPGA dispatch heuristics for rotate-based shifts.
Exposes the parallel-for work-group limit for heuristic calculations.
Adds range boundary handling and a targeted GPU test.

File summaries

File	Description
`algorithm_impl_hetero.h`	Adds rotate-based shift dispatch.
`parallel_backend_sycl_fpga.h`	Disables optimization for FPGA.
`parallel_backend_sycl_for.h`	Shares the work-group limit.
`glue_algorithm_ranges_impl.h`	Handles out-of-range shift values.
`shift_left_right.pass.cpp`	Adds a small-shift test case.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

danhoeflinger · 2026-09-08T21:03:27Z

+            __pattern_rotate(__tag,
+                             oneapi::dpl::__par_backend_hetero::make_wrapped_policy<__shift_via_rotate>(
+                                 std::forward<_ExecutionPolicy>(__exec)),
+                             __rng, static_cast<std::size_t>(__n));


I think this is acceptable with perhaps a note on deviation from standard. It is not out of the ordinary for parallel algorithms.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

danhoeflinger · 2026-09-17T13:03:38Z

@dmitriy-sobolev yes, sorry this should've bene in draft. Thanks for the review, I'll address your comments.

The gate carried a bare 'Empirically derived values'. Name the hardware and the element-size range the crossover was measured over, and say that 64 is the conservative end of the measured range rather than a fitted optimum. Comment-only: the code text of the file is byte-identical with comments stripped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The bound was 'n * sizeof(T) <= (min(max_work_group_size, 512) * max_compute_units) / 4', which compares a byte count against a work-item count and lets the divisor carry a hidden bytes-per-work-item conversion. max_work_group_size does not enter the mechanism at all: the walk's moves are dependent, so each of its n work items holds one load in flight and the walk has n * sizeof(T) bytes outstanding. Write it as '<= 128 * max_compute_units' instead, and say what the constant is. The 512 work-group limit binds on every device measured, so this is the same number (20,480 B on BMG, 131,072 B on PVC) and decides identically at all 715 grid points of the sweep on both GPUs; it differs only where max_work_group_size < 512, where the old form shrank the bound for a reason unrelated to memory saturation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

dmitriy-sobolev

Mostly cosmetic stuff

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

dmitriy-sobolev

LGTM

danhoeflinger and others added 12 commits September 4, 2026 10:26

gate for non positive or total shifts

288dd57

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

simplifying threshold logic for kernels

c7bc747

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

update thresholds with empirical data

f8fef3e

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

[shift] Adapt the ranges shift guard to main's parameter rename

d21b093

The guard added in fa6c034 referenced '__shift'; main renamed that parameter to '__n' in #2802, so the rebased branch did not compile.

[shift] Keep the rotate gate safe in a release build

16b7494

'assert' compiles away under NDEBUG, and the gate divides by '__n'. Callers do screen '0 < n < size', but the sentinel width of 0 also relies on n > 0, so degrade to the walk instead of dividing by zero. Add the missing <cassert>.

cleanup, simplify

e0b7ffd

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

remove unused function

41de574

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

improve comments

8fa6dd4

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

danhoeflinger requested a balanced review from Copilot September 8, 2026 20:49

Copilot started reviewing on behalf of danhoeflinger September 8, 2026 20:49 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

non power of 2 test

9fdb2a6

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

dmitriy-sobolev reviewed Sep 9, 2026

View reviewed changes

Comment thread include/oneapi/dpl/pstl/hetero/algorithm_impl_hetero.h Outdated

danhoeflinger marked this pull request as draft September 17, 2026 13:02

danhoeflinger and others added 7 commits September 17, 2026 09:24

improve comments, naming

783def8

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

address feedback

309863e

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

remove redundant end case checks

df0b5fd

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

address more feedback

eb7176a

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

fix clang formatting

3cb3259

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

danhoeflinger marked this pull request as ready for review September 17, 2026 17:31

danhoeflinger requested a review from SergeyKopienko September 17, 2026 17:32

dmitriy-sobolev reviewed Sep 18, 2026

View reviewed changes

Comment thread include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h Outdated

Comment thread include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_for.h Outdated

Comment thread include/oneapi/dpl/pstl/hetero/algorithm_impl_hetero.h Outdated

danhoeflinger added 3 commits September 18, 2026 10:45

address feedback

b38d36b

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

restore comment

be43112

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

reverting extra whitespace

6190d9b

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>

dmitriy-sobolev approved these changes Sep 18, 2026

View reviewed changes

danhoeflinger merged commit a53b01f into main Sep 21, 2026
25 checks passed

danhoeflinger deleted the dev/dhoeflin/autoperf/shift_reverse branch September 21, 2026 17:29

Navigation Menu

Fix shift_[left/right] for small shift values - #2824

Fix shift_[left/right] for small shift values#2824
danhoeflinger merged 23 commits into
mainfrom
dev/dhoeflin/autoperf/shift_reverse

danhoeflinger commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

danhoeflinger Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danhoeflinger commented Sep 17, 2026

Uh oh!

dmitriy-sobolev left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitriy-sobolev left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

danhoeflinger commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

danhoeflinger Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danhoeflinger commented Sep 17, 2026

Uh oh!

dmitriy-sobolev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitriy-sobolev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants