Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
⏳ Waiting for required checks to start or finish:
|
fcefa88 to
8ae7e0d
Compare
|
PR #14422 — "refactor(rocke): move convolution code from platform into library", author aledudek, branch What it does: moves convolution implementation (instances, C++ mirrors/headers/bindings, goldens, helpers, tests, docs) out of Introduces a new pattern: a Why: conv PRs kept needing How to apply: this directly extends the Code review (done 2026-10-27, against actual diff — fetched PR branch via
|
8ae7e0d to
57c1e3e
Compare
Thanks for the review, both points addressed, and the branch is rebased onto current develop.
Rebased onto develop (961dd76). Develop had restructured several things this PR touches, so the rebase re-applies the move to them rather than just resolving conflicts:
Byte-identity is GREEN, and every golden is OK on llvm20/22/23/24 with the same blessed and unblessed counts as develop. Commit structure is unchanged; commits 1 and 2 each build on their own. |
jakpiase
left a comment
There was a problem hiding this comment.
Review: blockers only
Verdict: approve with fixes. At its current base the move is clean. The blocker is the rebase onto develop. Paths below are relative to dnn-providers/hip-kernel-provider/rocke/.
At the current base (961dd76) the move is clean
Checked on CPU by building and running the gates:
- Pure move: git sees 162 renames, 14 adds and 68 modifies, and no file is deleted without a counterpart. The renamed C++ differs only in comments. All 77 test functions removed from platform exist in library tests.
- Python bindings: the 120 registered names are all still present (now 122; the new ones are
build_idandengine_version). - Golden split: across
ir_canonicaland all fourllflavours, the platform and library golden files together exactly equal the old platform file. Nothing is lost, duplicated or changed. - Build: works with
ROCKE_BUILD_PYBINDOFF and ON. A copy ofplatform/withoutlibrary/next to it still configures and builds. - Tests and gates: the CTest list is identical to base (46 tests, all pass).
check_byte_identityis GREEN on the same 95 families. 337 library and 428 platform CPU pytests pass.
Blocker: develop has added new conv code to platform/ since this PR's base
Seven files conflict, and that part is visible. The danger is that the new conv files merge with no conflict and stay in platform/. A rebase that just resolves the conflicts in the PR's favour then breaks the build or quietly loses test coverage:
-
The
rocke_corebuild breaks (reproduced with a trial merge).- Develop adds 8 conv files under
platform/cpp/:instances/common/conv_direct_dw_fwd_wmma.cpp,conv_direct_fwd_gstream_wmma.cpp,conv_direct_fwd_tapouter_wmma.cpp,conv_fwd_wmma_tdm.cppinclude/rocke/instance_conv_direct_dw_fwd_wmma.h,instance_conv_direct_fwd_gstream_wmma.h,instance_conv_direct_fwd_tapouter_wmma.h,instance_conv_fwd_wmma_tdm_internal.h
platform/CMakeLists.txt:160compiles everycpp/*.cppit finds, but the headers these files include now exist only underlibrary/cpp/include:instance_conv_direct_fwd_gstream_wmma.h:40:10: fatal error: rocke/instance_conv_direct_grouped.h: No such file or directorydw_fwd_wmmaandtapouter_wmmafail the same way.- Fix: move these 8 files into
library/cpp/{instances/common,include/rocke}.
- Develop adds 8 conv files under
-
Four test files can drop out of CTest without any error.
- Develop adds
test_direct_dw_fwd_wmma.py,test_direct_fwd_gstream_wmma.py,test_direct_fwd_tapouter_wmma.pyandtest_conv_fill.pyto the CTest lists inplatform/CMakeLists.txt, inside the conflicting blocks. - This PR moved those lists to
library/tests/ctest_lists.cmake, so keeping the PR side drops all four. - Fix: add them to
library/tests/ctest_lists.cmakein both the CPU and GPU lanes, then diff the installedCTestTestfile.cmakeagainst develop.
- Develop adds
-
Three new kernel families can lose golden gating while CI still passes.
- Develop adds golden entries to
platform/tests/instances/differential/golden/llvm_gfx_all.jsonforconv_direct_dw_fwd_wmma,conv_direct_fwd_gstream_wmmaandconv_direct_fwd_tapouter_wmma. It also re-blesses the llvm24 hashes forconv_direct_fwd_wmmaandconv_fwd_small_wmma. - After this PR, those families are checked against
library/tests/parity/golden/llvm_gfx_all.json, which git won't update because the PR creates it. - Configs with no blessed golden are reported as information only (
NEW (unblessed) … GOLDEN OK). If the entries are dropped from the platform file and never re-blessed into the library file, the three families are no longer gated and the check still passes. - Develop also adds:
- 3 golden files under
platform/tests/golden/rocke_representative_ir_sha256/conv_direct_*_wmma.json; - about 250 lines of conv cases in
rocke_ir_parity_harness.py.
- 3 golden files under
- Fix: move the golden entries into the library golden file, the 3 golden files into
library/tests/golden/conv_representative_ir/, and the parity cases intolibrary/tests/test_conv_ir_golden.py.
- Develop adds golden entries to
-
Develop's new conv Python and binding code must move too.
- Develop adds
conv_direct_{dw_fwd,fwd_gstream,fwd_tapouter}_wmma_spec_to_dictandlower_conv_direct_*toplatform/python/rocke/core/backend.py. - It also adds the matching
reg3/*_describebindings toplatform/cpp/bindings/rocke_engine.cpp. Those bindings include the headers moved in item 1. - Left in place,
ROCKE_BUILD_PYBIND=ONbreaks, and conv code ends up in platform again. - Fix: move them into
library/kernels/conv/engine_bridge.pyandlibrary/cpp/bindings/rocke_library_engine_conv.cpp.
- Develop adds
Merge condition: after the rebase, the heavy rocke CI (byte-identity and goldens on all LLVM flavours) must pass on the new head. Only the 5 lightweight checks have run so far.
Not verified
- The rebased head (it doesn't exist yet).
- GPU lanes.
- The full TheRock superbuild and wheel packaging.
- IR parity at llvm20/22/24 with the library tree in place (llvm23 was checked).
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical installation regressions leave convolution headers and symbols unavailable to C/C++ consumers and break installed test tooling.
8 open issues
Install and export the moved convolution API or provide compatibility · New Restrict the source-tree audit to source checkouts · New Stage _corpus.py for installed arch-domain tests · New Include the library corpus in the top-level validation gate · New Preserve the source package in the subprocess environment · New Skip compilation only when COMGR is unavailable · New Document PoolOp as a string-valued alias · New Match the pooling API's field names and string operation type · New
What changed in this PR
This PR separates convolution ownership from rocKE’s platform layer by relocating convolution C++ mirrors, tests, goldens and documentation into library/, while retaining shared platform infrastructure.
Changes:
- Adds a library C++ archive and companion
rocke_library_enginebindings. - Moves convolution validation assets into library-owned suites and extends IR gates to accept separate corpora.
- Updates helper imports, documentation references and platform dependency auditing.
| File | Description |
|---|---|
| dnn-providers/hip-kernel-provider/rocke/TESTING.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tools/check_ir_validity.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tools/check_byte_identity.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tools/check_arch_domain.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tools/_corpus.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_rocke_ci_static.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_library_reach.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_binding_compiler_selection.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/test_rocke_numeric.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/differential/test_check_byte_identity_progress.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/differential/golden/README.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_pipeline_basic.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_ir_serialize.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_arch_domain_lane.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/InstanceTests.cmake | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/__init__.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx1201/README.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx1201/ALGORITHM.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/README.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/optimization/measured_results.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/instances/small_ops.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/development/verify_dsl_docs.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/architecture/index.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/bindings/prove_parity_binding.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/bindings/CMakeLists.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/AGENTS.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_verify_dsl_docs_conv.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_fwd_small_wmma.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_direct_fwd_wmma.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_direct_dgrad_wmma.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_dgrad_stem_wmma.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_winograd_gen_cpu.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_wgrad_streamk.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_split_k.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_split_k_wgrad.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_split_k_planner.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_implicit_gemm_vector_width.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_implicit_gemm_numeric.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_fwd_group_merge_gate.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_direct_grouped_backend_parity.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_conv_dgrad_correctness.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_ceil_div_callers.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/run_all.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_winograd_gemm_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_winograd_fused_1d_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_wgrad_two_stage.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_fwd_split_k_reduce.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_direct_fwd_smallk_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_direct_fwd_smallc_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_dgrad_smallc_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/conv_representative_ir/conv_dgrad_patch_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/conv/docs/ck_tile_merged_groups.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/conv/__init__.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/README_conv_implicit_gemm_wgrad.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/README_conv_implicit_gemm_dgrad.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/README_conv_direct_nongrouped.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/README_conv_direct_grouped.md | 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_dgrad.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/conv_fwd_wmma_gate.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/conv_direct_grouped.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/dispatch/attention/common.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/tests/wgrad_streamk_host.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/tests/CMakeLists.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/instances/gfx1201/deep_fused_conv_pool_maxpool_gates_emitters.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/instances/gfx1201/deep_fused_conv_pool_conv1_gemm_lds_staging.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/instances/common/conv_implicit_gemm_wgrad.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/instances/common/conv_implicit_gemm_conv_compute_phase.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/instances/common/conv_fwd_wmma_gate.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_winograd_fwd_rt.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_fwd_wmma_gate_internal.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_fwd_split_k_reduce.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_direct_wgrad_gather.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_direct_dw_wgrad_mfma.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/include/rocke/instance_conv_abi.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/CMakeLists.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/winograd_glue.hpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine_winograd_gen.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine_winograd_gemm.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine_winograd_fwd.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine_winograd_fused1d.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/cpp/bindings/rocke_library_engine_img2col.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_wgrad/wgrad_split_k_planner_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_wgrad/wgrad_merged_groups_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_wgrad/wgrad_lds_layout_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_fwd/fwd_merged_groups_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/stride1_igemm_dgrad_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/README.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/grouped_generic_row_stream_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/dgrad_lds_layout_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/dgrad_fused_weights_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/dgrad_4c_bf16_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/conv_dgrad/depthwise_windowed_dgrad_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx1250/conv_kouter/gfx1250_transpose_read_case_study.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/benchmarks/gfx950/conv_dgrad/run_one_dgrad.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/benchmarks/common/split_k_cost_fit.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. |
| dnn-providers/hip-kernel-provider/rocke/KERNEL_AUTHORING.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/BUILDING.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/AGENTS.md | Updated as part of this pull request. |
Review effort: Max
| @dataclass(frozen=True) | ||
| class PoolingProblem: | ||
| N: int; Hi: int; Wi: int; C: int | ||
| Y: int; X: int # pool window | ||
| sH: int = 1; sW: int = 1 | ||
| pH: int = 0; pW: int = 0 | ||
| dH: int = 1; dW: int = 1 | ||
|
|
||
|
|
||
| class PoolOp(Enum): | ||
| MAX = "max" | ||
| SUM = "sum" | ||
| AVG = "avg" | ||
| ``` |
57c1e3e to
14245df
Compare
Thanks @jakpiase, this is exactly the trap: the new conv files merged cleanly and would have stayed in platform/. Rebased onto develop 0c4056e and moved everything #14835 added:
Gates on the rebased head: byte-identity GREEN (same as develop), ctest 46/46, golden and differential checks OK on all four flavors, and commits 1 and 2 each build on their own. The library CPU suites fail only the same gfx1250 Winograd build tests as develop (host toolchain). GPU lanes and the full superbuild still need CI. |
…and CI enrolment - platform/CMakeLists.txt adds ../library/cpp when present (rocke_library_core archive + rocke_library_engine pybind module, both linking rocke_core; nothing links back). -DROCKE_BUILD_LIBRARY_CPP=OFF skips it. - run_diff.py / check_byte_identity.py: library emitters compile against library/cpp/include and link librocke_library.a; families found under library/tests/parity are anchored in library/tests/parity/golden/llvm_gfx_all.json (entries moved verbatim, union unchanged); fails loudly if library C++ exists but its archive was not built. - Library CTest entries move to library/tests/ctest_lists.cmake (same installed tests). - rocke.core.backend publishes import_engine(module), lower_family, name_of. - tests/test_library_reach.py freezes platform -> library imports (shrink-only). - Docs: AGENTS.md, KERNEL_AUTHORING.md, TESTING.md, BUILDING.md, golden README.
- git mv of the conv_*, img2col and deep_fused_conv_pool instances and headers, including the gfx1250 WMMA and Winograd ones (no content change), into library/cpp; wgrad_streamk_host.cpp and conv_direct_dw_dgrad_host.cpp move to library/cpp/tests. Their pybind entry points, the four Winograd binding TUs among them, move out of rocke_engine into rocke_library_engine. - The conv spec->dict bridge and lower_conv_* / lower_img2col / lower_deep_fused_conv_pool / Winograd lowerings move from rocke.core.backend to library/kernels/conv/engine_bridge.py. - test_binding_compiler_selection uses a platform specimen (rmsnorm2d); prove_parity_binding resolves entry points from either module; the arch-domain binding scans in test_arch_domain_lane also read library/cpp/bindings when it is present. - Byte-identity unchanged: every family GREEN, conv emitters now link librocke_library.a.
…to library - Conv representative-IR cases move to library/tests/test_conv_ir_golden.py and their golden shards to library/tests/golden/conv_representative_ir/ (git mv, no re-bless); check_ir_validity / check_arch_domain gain --cases, and library/tests/run_all.py runs both over the conv corpus. - split_k_planner, the conv split-K selectors (select_split_k_wgrad, select_split_k_fwd, WgradSplitPolicy) and ConvKOuterFragmentReader move into library kernels. Platform's CU table becomes public (ARCH_NUM_CUS, DEFAULT_NUM_CUS, num_cus), so library callers no longer import its private names. - Conv tests from test_rocke.py, tests/core and test_rocke_numeric.py move to library tests and are enrolled in library/tests/ctest_lists.cmake. - convolution.md, ck_tile_merged_groups.md, the conv case studies and run_one_dgrad.py move into library; pooling docs stay in platform.
…lete Review follow-ups to the conv move: - The installed ckc package exports librocke_library.a as ckc::rocke_library (it links ckc::rocke_core) and installs library/cpp/include beside the engine headers, so the conv C API that shipped in rocke_core still ships under the same rocke/ include paths. - tools/run_checks.py runs the IR-validity and arch-domain gates over the library corpora (from library/tests/run_all.py) as well as the platform harness, so the conv cases stay in the top-level host gate. - _corpus.py is staged with the installed arch-domain tools, which import it through check_ir_validity.py. - test_library_reach.py audits only a source checkout; an installed test tree has no platform sources and stages the library under tests/. - test_conv_implicit_gemm_numeric.py launches through rocke_python_command / rocke_subprocess_env, as the platform test it came from does. - test_verify_dsl_docs_conv.py skips its comgr compile only when comgr cannot be loaded. - small_ops.md: the pooling sketch uses PoolingProblem's H/W fields and the string-valued PoolOp.
14245df to
31b2d97
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14422 — refactor(rocke): move convolution code from platform into library (aledudek) — 🟠 First-round: rebase must carry develop's work
Utility: 🟢 Positive — an exact move at its base; library owns conv end to end 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: trial merges with develop, head build + ctest, byte identity and goldens at 4 flavors, installed CTest lists and tests, code quality. Flags whether a closer look is needed; not an approval or rejection. |
|
Automated overlap notice (rocKE merge monitor) Hi @aledudek, this PR and #14464 both keep a C++ copy of the per-arch CU table and both add a public Python lookup for it to The copies, here at
All three C++ tables hold the same three rows and the same default of 256. Owner: #14464, as our review proposed and the monitor recorded. Platform code cannot use library code, so the one C++ table has to live in platform, and #14464 already puts it there: Order: the merge plan on develop
Rebase onto develop with #14464:
Both PRs also change 9 other files (among them Checked with If you think this PR should own it instead, reply here and on #14464, and the reviews will follow the new decision. Automated overlap check: one owner per shared piece of code, coordinated on the PRs. Nothing else changed. |
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:
For reassessment, the PR author should reply to top-level feedback or state that thread feedback is addressed. Before approval, top-level comments need explicit resolution from their author or that author's approval of the PR. |



Motivation
Convolution PRs keep needing @ROCm/rocke-platform review even when only conv code changes. Most recently that was #13993. The cause is that the conv C++ mirrors, the Python-to-C++ bridge, the IR goldens, the CTest enrolment and the conv docs all live under
rocke/platform/.This PR moves everything conv-owned that can reasonably move under
rocke/library/, which @ROCm/rocke-lib owns. Platform reviews the mechanism once. After that, conv-only changes touch onlylibrary/. Generic platform work (IR ops, shared helpers, arch data, lowering) stays in platform and is still reviewed there.The mechanism is not conv-specific. Attention/SDPA, whose C++ mirrors also live in
platform/cpp, can use it next.Technical Details
The PR has three commits, meant to be reviewed in order.
1. Enablers. No conv code moves in this commit.
platform/CMakeLists.txtadds../library/cppwhen that directory exists. It builds two targets:rocke_library_core(librocke_library.a);rocke_library_engine.Both link
rocke_core; nothing in platform links back.-DROCKE_BUILD_LIBRARY_CPP=OFFskips them, and a copy ofplatform/alone still configures and gates.run_diff.pyandcheck_byte_identity.py: library parity emitters compile againstlibrary/cpp/includeand linklibrocke_library.a.library/tests/parityare anchored inlibrary/tests/parity/golden/llvm_gfx_all.json. Their entries were moved verbatim, so the union of the two golden files is unchanged.The library CTest entries move from
platform/CMakeLists.txtintolibrary/tests/ctest_lists.cmake. The installed test list is unchanged.rocke.core.backendpublishesimport_engine(module),lower_familyandname_of, so library bridges reuse the same python/cpp/both dispatch.New
platform/tests/test_library_reach.pyfreezes platform's imports of library packages. It is shrink-only.AGENTS.md,KERNEL_AUTHORING.md,TESTING.mdandBUILDING.mddocument the new layout.2. C++ mirrors.
conv_*,img2colanddeep_fused_conv_poolinstances and headers, pluswgrad_streamk_host.cpp, move withgit mvtolibrary/cpp/{instances,include,tests}. File contents are unchanged and therocke/include prefix is kept.rocke_engineintorocke_library_engine.lower_conv_*,lower_img2colandlower_deep_fused_conv_poolfunctions move fromrocke.core.backendtolibrary/kernels/conv/engine_bridge.py.3. Goldens, helpers, tests and docs.
library/tests/test_conv_ir_golden.pyandlibrary/tests/golden/conv_ir_sha256.json. The entries were moved verbatim, not re-blessed.check_ir_validity.pyandcheck_arch_domain.pygain--cases <file>.library/tests/run_all.pyruns both over the conv cases, so their coverage is kept.select_split_k_wgradandConvKOuterFragmentReadermove into library kernels.test_rocke.py,tests/coreandtest_rocke_numeric.pyinto library tests, and are enrolled inctest_lists.cmake.convolution.md,ck_tile_merged_groups.md, the conv case studies andrun_one_dgrad.pymove into library. The pooling docs stay in platform (small_ops.md).Removed platform API (new homes are listed above):
rocke_engine;rocke.core.backend.lower_conv_*and*_spec_to_dict;rocke.helpers.split_k_planner;rocke.helpers.split_k.select_split_k_wgrad;rocke.helpers.ConvKOuterFragmentReader.Open conv branches that touch the moved files need a rebase. Their golden changes must be re-blessed into the new library golden files rather than merged.
Test Plan
Each item was run after every commit and compared against
developat the same base:platform/tools/check_byte_identity.py, the byte-identity gate between the Python and C++ engines;rocke_ir_parity_harness.py --check(platform) andtest_conv_ir_golden.py(library), each on every LLVM flavor;check_ir_validity.pyandcheck_arch_domain.pyover the platform corpus, and with--casesover the conv corpus;ctestfor C++ host tests, including the movedrocke_wgrad_streamk_host;pytest(-m "not integration") and the library conv CPU tests, with failure sets diffed againstdevelop;backend="both"lowering of every conv parity config, before and after the move;library/beside it.Test Result
librocke_library.a.develop. Every test moved out of platform passes in library.Submission Checklist