Skip to content

Batch compile_ops across all modules from the root module - #5370

Open
pfultz2 wants to merge 5 commits into
developfrom
compile-ops-submodules
Open

pfultz2 wants to merge 5 commits into
developfrom
compile-ops-submodules

Conversation

@pfultz2

@pfultz2 pfultz2 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Motivation

compile_ops ran once per module, so each submodule compiled and benchmarked its own precompile ops in isolation. Identical kernels in different submodules only avoided a second compile by hitting the in-memory binary cache after the first module finished, and the parallel compile was split into many small per-module batches instead of one large one.

Technical Details

  • compile_ops::apply now returns immediately for non-root modules. On the root it collects the root plus every reachable submodule, deduplicated and skipping bypassed modules, matching what the pass manager skips.
  • compile_manager gained add_plans(ctx, module), and compile() no longer takes a module or is_root flag. Each compile_plan already records its owning module, so results are replaced into cp.mod.
  • replace_inserted_device_ops takes a list of modules. It runs dead_code_elimination and lower_device_ops on each, then compiles the inserted device ops in one batch as well. The benchmark MXR dump path passes a single-module list.
  • Added same_kernel_in_two_submodules_compiles_once to test/gpu/binary_cache.cpp. An if program with the same pointwise kernel in both branches must produce one code object per submodule with exactly one compile and zero cache reuses, which pins the batched behavior rather than the old compile-then-reuse sequence.

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

    • Added: New functionality.
    • Changed: Changes to existing functionality.
    • Removed: Functionality or support that has been removed. (Compared to a previous release)
    • Optimized: Component performance that has been optimized or improved.
    • Resolved Issues: Known issues from a previous version that have been resolved.
    • Not Applicable: This PR is not to be included in the changelog.

Follow the LLVM AI Tool Use Policy for contributions using AI.

Copilot AI balanced review requested due to automatic review settings October 8, 2026 21:28
@pfultz2
pfultz2 requested a review from causten as a code owner October 8, 2026 21:28
@pfultz2
pfultz2 requested review from ahsan-ca and eddieliao and a balanced review from Copilot and removed request for causten and Copilot October 8, 2026 21:28

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

Nested compilation exits before benchmark MXR files can be saved when dump mode is enabled.

2 open findings
What changed in this PR

Batches GPU operation compilation across the root module and reachable submodules to improve kernel deduplication and parallelism.

Changes:

  • Collects eligible modules and compiles their plans together.
  • Batches compilation of inserted device operations.
  • Adds cross-submodule binary-cache coverage.

Review was performed as a single pass without agent fan-out.

File Description
src/​targets/​gpu/​compile_ops.cpp Implements cross-module compilation batching.
test/​gpu/​binary_cache.cpp Tests deduplication across conditional submodules.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/targets/gpu/compile_ops.cpp Outdated

// Exit on the root module so all submodules get processed first.
if(dump_mxr and is_root)
if(dump_mxr)
Comment thread test/gpu/binary_cache.cpp
EXPECT(cache->get_stats().reused == 1);
}

static migraphx::program same_pointwise_in_two_submodules()
@codecov

codecov Bot commented Oct 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5370      +/-   ##
===========================================
- Coverage    92.71%   92.70%   -0.00%     
===========================================
  Files          635      635              
  Lines        36204    36204              
===========================================
- Hits         33563    33562       -1     
- Misses        2641     2642       +1     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants