Skip to content

fix(rocke): shift HIP vector lshr lanes in their own width - #14844

Merged
cgmillette merged 2 commits into
developfrom
users/cgmillette/rocke/hip-vector-lshr
Nov 2, 2026
Merged

cgmillette merged 2 commits into
developfrom
users/cgmillette/rocke/hip-vector-lshr

Conversation

@cgmillette

Copy link
Copy Markdown
Contributor

Motivation

On the HIP route, vector.lshr on i8 or i16 lanes gives wrong results for any lane with its top bit set: a lane holding -128 shifted right by 2 gives -32 instead of 32. The LLVM route gives the right result, so the two compile routes disagree on the same kernel.

Technical Details

Both HIP lowerings (Python _Lowerer._op_vector_lshr, C++ rocke_h_op_vector_lshr) emitted res[i] = ((uint32_t)a[i]) >> b[i]; for every element type. For an i8 or i16 lane the cast sign-extends a negative value to 32 bits first, so the shift moves sign bits into the result, where LLVM's lshr <N x iW> shifts in zeros at bit W-1.

Both engines now read each lane through the unsigned type of its own width (uint8_t, uint16_t, uint32_t), looked up in the existing unsigned-type table (_HIP_UNSIGNED_INT_TYPE / rocke_h_unsigned_int_type); an element with no entry is refused. An i32 lane still reads as uint32_t, so its source is unchanged. Only the HIP lowering changes; the LLVM IR of every kernel is unchanged.

Test Plan

  • New platform/tests/core/test_vector_lshr.py, for 4 x i8, 2 x i16 and 2 x i32 lanes:
    • both engines emit the same HIP source, and every lane is read through the unsigned type of its width;
    • hipcc compiles that source for gfx942 and gfx950;
    • on a GPU, the LLVM route and hipcc on the both-engine HIP source each match a numpy logical shift, with negative and positive lanes in the input.
  • The same test against develop.
  • Byte identity at llvm20, llvm22, llvm23, llvm24; ctest; HIP output of every parity-emitter config with develop and with this branch, both engines; full platform suite (including goldens) against develop; pre-commit.

Test Result

  • New test: 11 passed with both engines (strict), the GPU cases on a local gfx90a. On develop the i8 and i16 HIP source checks and the i8 and i16 hipcc GPU runs fail (4 failed, 7 passed); for 4 x i8, 114 of 256 lanes differ from numpy.
  • Byte identity GREEN at llvm20, llvm22, llvm23 and llvm24.
  • HIP output: 803 parity-emitter kernels, text and refusals identical on develop and this branch in the Python engine (765 lowered, 38 refused) and in the C++ engine (524 lowered, 279 refused).
  • ctest 35/35.
  • Full platform suite (platform/tests): 6232 passed, 769 skipped, 2 failed; develop in the same environment: 6221 passed, 769 skipped, the same 2 failed (test_arch_domain_lane.py::test_the_cpp_fallback_does_not_warn_twice, test_binding_compiler_selection.py::test_python_bindings_use_the_compiling_library). The 11 extra passes are the new test.
  • Pre-commit and ruff check clean.

ISSUE ID : AICK-2902

🤖 Generated with Claude Code

The HIP lowering of vector.lshr read every lane as uint32_t before
shifting. For an i8 or i16 lane that sign-extends a negative value first,
so its sign bits were shifted into the result, where LLVM's lshr shifts in
zeros. Both engines now read each lane through the unsigned type of its
own width. An i32 lane still reads as uint32_t, so its source is
unchanged.

A new test checks that both engines emit the same HIP source with the
unsigned read, compiles that source with hipcc for gfx942 and gfx950, and
on a GPU compares the LLVM route and hipcc on that source with numpy, on
negative and positive i8, i16 and i32 lanes.

Co-Authored-By: Claude <noreply@anthropic.com>
@cgmillette
cgmillette requested a review from a team as a code owner November 1, 2026 22:15
@therock-pr-bot

therock-pr-bot Bot commented Nov 1, 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 1, 2026

Copy link
Copy Markdown

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

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14844 — shift HIP vector lshr lanes in their own width (cgmillette, AICK-2902) — 🟡 First-round: verified; CI skips HIP cases

Prior item Status
Reviewers 🟢 None yet
• no reviews or threads yet
Body: HIP vector.lshr sign-extends i8/i16 lanes 🟢 FIXED c82f7721
• both engines: same HIP, lanes read as uint8_t/uint16_t/uint32_t (ran)
• gfx1151 GPU, HIP source via clang: develop 114/256 i8 lanes wrong, merge 0 (ran)
• not covered: scalar arith.lshr (i8/i16/i64) keeps (unsigned) (ran)
Body: HIP output of 803 parity kernels unchanged 🟢 Reviewed
• 10 at-risk families, 128 configs: identical, both engines (ran)
• none builds a vector.lshr; all callers use i32 lanes (read)
Bots / CI 🟡 running
• fails: gfx94X hipkernelprovider shard 1/2 — golden sha256 test timeout, develop issue 14733
• fails: Multi-Arch CI Summary — first run, cancelled by a label re-run
• queued: Windows gfx110X sanity; 46 ✓
Our checks 🟢 Clean
• merge vs 61b3ac0e: clean; rules 1–3: 1 text-I/O hint, ASCII-only (ran)
• build + ctest 34 ✓ / 1 skip; byte identity ×4, goldens, differential ✓ (ran)
• new test: merge 6 ✓ / 5 skip (host hipcc); develop 2 ✗ (ran)
# Sev file:line Finding
N1 🟡 platform/tests/core/test_vector_lshr.py:72, :84 All 8 HIP cases need rocke_engine, which CI lacks
• impact: gfx94X CI ran only the 3 LLVM-route cases; a revert stays green (read)
• fix: Python-only HIP when the engine is absent; CI has hipcc and a GPU

Utility: 🟢 Positive — HIP route now matches LLVM's lshr on i8/i16 lanes; nothing else changes.


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, differential golden, the new test with the native engine on develop and the merge, its HIP source on a gfx1151 GPU, HIP output of the at-risk parity configs, CI at head. Flags whether a closer look is needed; not an approval or rejection.

@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. I agree with the following existing comments:

  • HIP regression tests skip without rocke_engine — cgmillette — #14844 (comment)

@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
cgmillette merged commit aa5f685 into develop Nov 2, 2026
163 of 166 checks passed
@cgmillette
cgmillette deleted the users/cgmillette/rocke/hip-vector-lshr 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.

2 participants