Skip to content

feat(rocke): disable unroll k for basic pipeline - #12409

Merged
bartekxk merged 4 commits into
developfrom
users/barkocot/disable-basic-unroll-k
Sep 24, 2026
Merged

bartekxk merged 4 commits into
developfrom
users/barkocot/disable-basic-unroll-k

Conversation

@bartekxk

@bartekxk bartekxk commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Speedup compilation time

Technical Details

Disable unroll k for basic pipeline in implicit gemm conv.

Test Plan

test_conv_*_corectness.py

Test Result

Passed

Submission Checklist

JIRA ID: AICK-2217

@bartekxk bartekxk self-assigned this Sep 22, 2026
@aledudek
aledudek force-pushed the users/aledudek/rocke-wgrad-group-merge branch 2 times, most recently from b742a9b to 23bb138 Compare September 22, 2026 14:17
Base automatically changed from users/aledudek/rocke-wgrad-group-merge to develop September 22, 2026 21:53
@therock-pr-bot

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

Copy link
Copy Markdown

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

@bartekxk
bartekxk marked this pull request as ready for review September 23, 2026 08:10
@bartekxk
bartekxk requested review from a team as code owners September 23, 2026 08:10
@bartekxk
bartekxk requested a lite review from Copilot September 23, 2026 08:11

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.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 High severity · 1 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Updates ROCm’s rocKE implicit-GEMM convolution codegen to use runtime K-loops for the basic pipeline, while adding grouped wgrad split-K tuning and depthwise group merging.

Changes:

  • Replaces basic-pipeline Python unrolling with bounded runtime loops in Python and C++.
  • Adds groups-, block-size-, XCD-, and grid-limit-aware split-K selection.
  • Adds group-merge codegen, validation, workspace handling, benchmarks, and tests.
File Description
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​rocke_ir_parity_harness.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​dispatch/​dispatch_tests/​gemm/​test_split_k_wgrad.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​helpers/​split_k.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​instances/​common/​conv_implicit_gemm_wgrad.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​instances/​common/​conv_implicit_gemm_conv_build_glue.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​tests/​test_conv_wgrad_correctness.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​tests/​dispatch/​test_grouped_conv_wgrad_dispatch.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​kernels/​common/​conv_implicit_gemm.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​kernels/​common/​conv_implicit_gemm_wgrad.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​kernels/​common/​conv_implicit_gemm_wgrad_two_stage.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​dispatch/​grouped_convolution.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​library/​benchmarks/​common/​benchmark_implicit_gemm_conv.py Updated as part of this pull request.

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

@bartekxk
bartekxk force-pushed the users/barkocot/disable-basic-unroll-k branch from 16acaf7 to 8833eff Compare September 23, 2026 12:18
@bartekxk
bartekxk force-pushed the users/barkocot/disable-basic-unroll-k branch from 8833eff to 5c8430b Compare September 23, 2026 12:44
@cgmillette

Copy link
Copy Markdown
Contributor

PR #12409 — Reviewed with Claude 4.7

🟡 = should change
⚪ = suggestion

# Sev Where Problem Fix
1 🟡 kloop drivers pipeline="basic" loses its VMEM/compute overlap — now emits the plain mem loop. Runtime codegen changes, not just compile time. Confirm no perf regression on shapes that used basic.
2 🟡 wgrad ctx-init Newly allows wgrad basic+split_k=0 (runtime trip count via kloop_num_iters=0); no test covers it. Add build+numeric test for that combo.
3 ⚪ glue/docs basic is now ~identical to mem (schedule hints only). Doc as alias or deprecate.
4 ⚪ C++ tree Verify no leftover refs to deleted rocke_conv_emit_kloop_basic / _global_read / _lds_write / _MAX_BASIC_K_ITERS. Grep to confirm build.
5 ⚪ test_pipeline_basic.py Assertion changed to exact op.name=="scf.for" (was substring "for_iter"). Confirm op name matches.

Verdict: PASS — 0 blocking. Clean refactor, Python↔C++ parity kept, goldens/test/docs updated together.

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

LGTM

@bartekxk
bartekxk merged commit b7d6aec into develop Sep 24, 2026
92 checks passed
@bartekxk
bartekxk deleted the users/barkocot/disable-basic-unroll-k branch September 24, 2026 09:32
@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.

4 participants