Skip to content

Fix scanning out-of-bounds items in OpenMP scan#9759

Open
bernhardmgruber wants to merge 2 commits into
NVIDIA:mainfrom
bernhardmgruber:fix_omp_scan
Open

Fix scanning out-of-bounds items in OpenMP scan#9759
bernhardmgruber wants to merge 2 commits into
NVIDIA:mainfrom
bernhardmgruber:fix_omp_scan

Conversation

@bernhardmgruber

Copy link
Copy Markdown
Contributor

Fixes: #9725

@bernhardmgruber bernhardmgruber requested a review from a team as a code owner July 9, 2026 08:46
@bernhardmgruber bernhardmgruber requested a review from miscco July 9, 2026 08:46
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Jul 9, 2026
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a73f8b85-9818-47e1-bedb-d562bc431c8a

📥 Commits

Reviewing files that changed from the base of the PR and between d5fc08b and 3078353.

📒 Files selected for processing (1)
  • thrust/thrust/system/omp/detail/scan.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • thrust/thrust/system/omp/detail/scan.h

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved parallel scan reliability by allocating and running work based on the actual number of active threads needed for the input size.
    • Simplified per-thread bounds handling while preserving the existing inclusive/exclusive scan semantics, including the special-case behavior for the no-initial inclusive scan.

Walkthrough

OpenMP scan now derives active_threads from the input size, uses it to size block_sums, and applies it to both parallel regions while removing the prior start < n guards.

Changes

OMP scan fix

Layer / File(s) Summary
Block-sum reduction sizing
thrust/thrust/system/omp/detail/scan.h
block_sums is sized to active_threads, and the block reduction region runs with num_threads(active_threads) after removing the start < n guard.
Per-block scan with prefix offsets
thrust/thrust/system/omp/detail/scan.h
Step 3 runs with num_threads(active_threads) and drops the start < n guard while keeping the existing inclusive/exclusive and no-init prefix behavior.

Assessment against linked issues

Objective Addressed Explanation
Fix assertion failure in thrust.cpp.omp.test.scan due to invalid data reads [#9725]

Out-of-scope changes


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
thrust/thrust/system/omp/detail/scan.h (1)

115-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

suggestion: first_elem is never mutated; mark it const. As per coding guidelines, "All variables that are not modified must be declared const".

-    accum_t first_elem = *(first + start);
+    const accum_t first_elem = *(first + start);

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cff36059-3fd5-4e46-b97b-ceb100b7d0e9

📥 Commits

Reviewing files that changed from the base of the PR and between f51b10a and d5fc08b.

📒 Files selected for processing (1)
  • thrust/thrust/system/omp/detail/scan.h

@github-actions

This comment has been minimized.

@Jacobfaib

Copy link
Copy Markdown
Contributor

Sorry fat-fingered approval before adding comment. Could you add a test that exercises the bug?

@bernhardmgruber

Copy link
Copy Markdown
Contributor Author

Sorry fat-fingered approval before adding comment. Could you add a test that exercises the bug?

The existing test covers the bug if run with a CPU of at least 128 threads :)

@github-actions

Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 23h 46m: Pass: 100%/119 | Total: 1d 02h | Max: 1h 08m | Hits: 69%/252077

See results here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

thrust.cpp.omp.test.scan asserts in CCCL 13.4

3 participants