Skip to content

feat(rocke): add scalar optimization barrier APIs - #12165

Merged
tenpercent merged 12 commits into
developfrom
users/mpodkory/rocke/optimization-barrier
Oct 10, 2026
Merged

tenpercent merged 12 commits into
developfrom
users/mpodkory/rocke/optimization-barrier

Conversation

@tenpercent

@tenpercent tenpercent commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

ISSUE ID : AICK-2253

Kernel authors sometimes need an intermediate floating-point result to round before the next operation. Combining a multiply with an add or FP16 conversion can change that result.

Add IRBuilder.optimization_barrier(value) and rocke_b_optimization_barrier(builder, value). They return the same scalar value and type while preventing the compiler from combining operations across the returned value. Both builders use existing empty tied inline assembly; booleans and byte-sized values bridge through i32. Packed logical formats use their integer storage/carriers.

This PR covers the AMDGPU LLVM IR and COMGR compilation path. It includes Python/C++ parity emitters, native scalar admission and serialization/LLVM tests, committed LLVM and canonical-IR goldens, and GPU checks for bit preservation, lane predicates, and intermediate rounding. Native HIP-source lowering remains unsupported; existing Python HIP lowering is unchanged from develop.

The numeric suite uses COMGR directly and does not invoke hipcc. The native test runs as a standalone CTest, so the barrier-specific executable-discovery fixture, bare import conftest calls, and fixture .__wrapped__() access are removed.

Test plan and validation on f89b9389a7d8:

  • 16/16 native CTests passed, including the installed provider-named barrier executable. The retained Python barrier suite passed all 21 cases with strict Python/C++ comparison.
  • The installed provider-layout full Python suite passed: 1,947 tests and 1,632 subtests; 454 capability/dependency skips.
  • Python/C++ LLVM parity passed all 76 families / 957 configurations for each of LLVM 20, 22, and 23. All seven barrier verifier configurations passed. All seven committed LLVM 20 and canonical-IR barrier goldens matched; the builders, parity emitters, and golden file are unchanged by this scope fix.
  • gfx942 COMGR GPU validation passed all 12 cases with zero skips on ROCm 7.14 / LLVM 23 (Slurm job 68057070). This GPU run used the Python LLVM engine; native/Python LLVM equality is established separately by the parity gates.
  • Top-level hooks over the committed PR range and git diff --check passed. The full relative-path guard flags the repository-relative bundle path in cmake/InstallGpuReferences.cmake:30; that file has the same blob at the develop base and this head, so the guard failure is inherited.

@therock-pr-bot

therock-pr-bot Bot commented Sep 16, 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 Sep 16, 2026 •

Copy link
Copy Markdown

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

Copilot AI 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.

🟡 Changes recommended

The C API accepts unsupported scalar types and fails HIP lowering for barriers supported by Python.

2 open findings
What changed in this PR

Adds scalar optimization barriers to rocKE’s Python and C builder APIs to preserve intermediate rounding boundaries.

Changes:

  • Adds value-preserving barriers, widening predicates and byte-sized values through i32.
  • Adds parity, serialization, bit-preservation, and rounding tests.
  • Documents supported types and differences from scheduling barriers.

Paths below are relative to dnn-providers/hip-kernel-provider/rocke/.

File Description
platform/​tests/​instances/​test_optimization_barrier_numeric.py Tests GPU bit preservation and rounding.
platform/​tests/​instances/​parity/​optimization_barrier_emit.py Adds Python parity emitter.
platform/​tests/​instances/​parity/​optimization_barrier_emit.c Adds native C parity emitter.
platform/​tests/​core/​test_optimization_barrier.py Tests serialization, lowering, and input rejection.
platform/​python/​rocke/​core/​ir.py Implements Python barrier API.
platform/​dsl_docs/​reference/​api_index.md Links the barrier guide.
platform/​dsl_docs/​primitives/​intrinsics_and_primitives.md Explains barrier usage and limitations.
platform/​cpp/​include/​rocke/​ir.h Declares C barrier API.
platform/​cpp/​core/​ir/​ir_tile.cpp Implements C barrier builder.

🧠 Review effort: Balanced


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

Copilot AI 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.

🟡 Changes recommended

Malformed attribute validation and committed golden baselines remain unresolved.

1 open finding
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add optimization barrier golden hashes for new architectures

dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​parity/​optimization_barrier_emit.py:36

Rule 10: Neither new architecture configuration has an optimization_barrier entry in tests/instances/differential/golden/llvm_gfx_all.json. The golden checker treats unblessed configurations as informational, so parity alone leaves these cases without a fixed emission baseline. Add reviewed LLVM and canonical-IR golden hashes for both configurations, preserving existing entries.

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

Compiler-sensitive rounding and bit-preservation guarantees across both engines warrant final human validation.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@tenpercent
tenpercent marked this pull request as ready for review October 7, 2026 21:27
@tenpercent
tenpercent requested review from a team as code owners October 7, 2026 21:27

@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 findings. The PR is good to go for your approval.

@tenpercent
tenpercent requested a review from a team October 9, 2026 05:01

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

PR #12165 — scalar optimization barrier (tenpercent, AICK-2253) — 🟢 ACCEPT: after a small rebase

Prior item Status
Copilot: C++ HIP inline asm = NOTIMPL 🟢 FIXED — tied-VGPR barrier lowered (lower_hip_mma.cpp:219)
Copilot: C++ admits fp4/fp6/e8m0/e5m3 🟢 FIXED — range ends at BF8E5M2 (ir_tile.cpp)
Copilot: malformed asm attrs accepted 🟢 FIXED — bool/string kinds enforced
Copilot: no barrier goldens 🟢 FIXED — ll + canonical for gfx950/gfx1250
CongMa13: no findings 🟢 info
# Sev file:line Finding
N1 🟠 run_all.py, test_run_all.py Dirty vs #12957; overlaps open #13352 in native_pytest_env → rebase
N2 🟡 intrinsics_and_primitives.md "=v,0" makes uniform values divergent; doc doesn't warn

Utility: 🟢 Positive — small, mirrored rounding barrier; well tested.


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

@tenpercent

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Co-authored-by: tenpercent <4273004+tenpercent@users.noreply.github.com>

Copilot AI commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved in e55b104 by merging current develop, preserving upstream native-fixture discovery and barrier coverage. GitHub no longer reports merge conflicts. Validation passed: 23 runner tests, 16 native CTests, 117 Python regressions (33 optional-tool skips), barrier parity for LLVM 20/22/23, PR hooks, and secret scan. Focused review found no issues; automated review/security validation timed out twice on the large merge diff.

Copilot AI 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.

🔵 Needs a closer look

Low-level dual-engine compiler lowering and hardware-dependent rounding semantics warrant final human review.

1 open finding

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

The architecture-neutral API lacks parity and golden validation for four shipped targets.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add parity and golden coverage for all supported targets

dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​parity/​optimization_barrier_emit.py:36

Rule 8 — A change inside shared structure reaches every member: this arch-neutral barrier API/lowering is reachable on every shipped target, but the parity family covers only gfx1250 and gfx950. core/arch/data/arch_specs.json:6-152 also registers gfx90a, gfx942, gfx1151, and gfx1201, and the public docs impose no target restriction, so those targets receive neither Python/C++ parity nor golden validation. Add configurations and golden entries for the remaining supported targets, or explicitly reject and document them as unsupported.

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

Compiler-optimization semantics across multiple LLVM and HIP targets warrant final human validation despite comprehensive tests.

0 open findings

🧠 Review effort: Balanced

@tenpercent
tenpercent enabled auto-merge (squash) October 9, 2026 21:54

Copilot AI 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.

🔵 Needs a closer look

The cross-engine compiler barrier semantics and multi-architecture GPU behavior warrant final human review despite extensive coverage.

0 open findings

🧠 Review effort: Balanced

@tenpercent
tenpercent merged commit 01d971c into develop Oct 10, 2026
95 of 96 checks passed
@tenpercent
tenpercent deleted the users/mpodkory/rocke/optimization-barrier branch October 10, 2026 03:20
@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.

5 participants