Skip to content

docs(rocke): say what a taken trap does on each platform - #14866

Merged
cgmillette merged 1 commit into
developfrom
users/cgmillette/rocke/trap-docs
Nov 2, 2026
Merged

cgmillette merged 1 commit into
developfrom
users/cgmillette/rocke/trap-docs

Conversation

@cgmillette

Copy link
Copy Markdown
Contributor

Motivation

The docs for trap() said a taken trap makes the launch report an error or abort the process, and that it lowers to s_trap 2. Neither holds everywhere, so a kernel author reading them could expect a recoverable error where there is none.

Technical Details

Docs and comments only; no emitted IR or HIP source changes.

  • On Linux (ROCr) the queue reports an error and the process aborts.
  • On Windows the HIP runtime never turns the trap into an error and the launch never returns.
  • Under a debugger the wave stops at the trap.
  • On gfx11 the AMDGPU backend emits its simulated trap: s_trap 2, then s_sendmsg, then an s_sethalt loop. Other targets get s_trap 2.

Updated: the IRBuilder.trap docstring, the tile.trap entry in dsl_docs/reference/op_vocabulary.md, the rocke_b_trap comment in cpp/include/rocke/ir.h, and the module docstring of tests/core/test_trap.py. Each now says that a check the process must survive belongs on the host, before the launch.

Test Plan

  • Engine and bindings build; ctest.
  • tools/check_byte_identity.py (default flavor plus the gfx1250 llvm23 pass).
  • tests/core/test_trap.py with both engines.
  • pre-commit over the branch.
  • The gfx11 and other-target trap sequences were checked by compiling an llvm.trap kernel with llc for gfx90a, gfx942, gfx950, gfx1100 and gfx1201.

Test Result

All green; byte identity GREEN for every family.

Submission Checklist

ISSUE ID : AICK-2906

🤖 Generated with Claude Code

The trap docs said a taken trap makes the launch report an error or abort
the process, and that it always lowers to s_trap 2. Neither is true
everywhere. On Linux (ROCr) the queue reports an error and the process
aborts; on Windows the HIP runtime never turns the trap into an error and
the launch never returns; under a debugger the wave stops at the trap. On
gfx11 the backend emits its simulated trap: s_trap 2, then s_sendmsg, then
an s_sethalt loop.

Correct the IRBuilder.trap docstring, the op vocabulary entry, the C
builder's header comment and the trap test's module docstring, and say
that a check the process must survive belongs on the host.

Co-Authored-By: Claude <noreply@anthropic.com>
@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@CongMa13 CongMa13 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.

No new issues found.

@CongMa13 CongMa13 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.

No new P0 or P1 issues were identified during this review. The changes are approved.

@cgmillette

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14866 — say what a taken trap does on each platform (cgmillette, AICK-2906) — 🟡 First-round: gfx11 overstated; CI running

Prior item Status
CongMa13 (11-02) APPROVED: no P0/P1 issues 🟢 APPROVED bb8d18b8
• at this head; no items raised
Bots / CI 🟡 Running
• running: Multi-Arch CI re-run after the org label; 3 jobs
• fails: Multi-Arch CI Summary — cancelled run's, re-run pending (read)
• 30 ✓ incl. hipDNN superbuilds · 6 cancelled
Our checks 🟢 Clean
• merge vs 2a155d53 clean; rocke/ tree = head's (ran)
• build + ctest 34 ✓ / 1 skip; byte identity GREEN ×4 flavors (ran)
• goldens PASS, 0 changed; code tokens unchanged; test_trap 6 ✓ (ran)
# Sev file:line Finding
N1 🟡 ir.py:5527, op_vocabulary.md:196, ir.h:1813, test_trap.py:7 "gfx11" is too broad: the simulated trap is gfx1100–gfx1103 only
• impact: gfx1150–gfx1153 (e.g. gfx1151) emit plain s_trap 2 (ran)
• Windows hang evidence is gfx110X CI only (read)
• fix: write gfx110x (gfx1100–gfx1103) in all four files

Utility: 🟢 Positive — corrects per-platform trap docs; comments only, IR unchanged.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

First-round review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop, build + ctest, byte identity, goldens, comment-only token diff, test_trap, trap ISA per target, CI at head. Flags whether a closer look is needed; not an approval or rejection.

@cgmillette
cgmillette merged commit 4203e62 into develop Nov 2, 2026
142 of 149 checks passed
@cgmillette
cgmillette deleted the users/cgmillette/rocke/trap-docs branch November 2, 2026 19:52
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants