Skip to content

refactor(ir): extract LoweringBuilder from lower_composite_ops_pass (plan 70, 1/3) - #2792

Open
georgebisbas wants to merge 1 commit into
hw-native-sys:mainfrom
georgebisbas:refactor/split-lower-composite-ops
Open

georgebisbas wants to merge 1 commit into
hw-native-sys:mainfrom
georgebisbas:refactor/split-lower-composite-ops

Conversation

@georgebisbas

Copy link
Copy Markdown
Contributor

Phase 1 of 3 for plan 70 — splitting lower_composite_ops_pass.cpp into per-collective
translation units. Mirrors public issue #2632, which proposes the same split.

Why

lower_composite_ops_pass.cpp had grown to 2845 lines holding every composite collective
lowering rule in one translation unit. 13 commits touched it in the last 60 days, so each new
collective lands hundreds of lines in the same file and any edit risks every other rule.
Plan 36 proposed reordering the pipeline instead; that was rejected in #1850. This is the
structural split without a reorder.

What this PR does

Moves the shared LoweringBuilder scratchpad, its CommSetup result struct and the
MakeNegation helper into a new src/ir/transforms/lower_composite/ module, so the
per-collective rules extracted in the follow-ups have a shared home to depend on.

Pure code motion — the rules still resolve LoweringBuilder and CommSetup through using
declarations, so there are no call-site changes and no IR output changes.
lower_composite_ops_pass.cpp: 2845 → 2356 lines.

Stack

This is the first of three; they must land in order.

Branch Contents
1 refactor/split-lower-composite-ops this PRLoweringBuilder extract
2 refactor/split-lower-composite-ops-pr2 shared helpers, allreduce (mesh+ring), allgather
3 refactor/split-lower-composite-ops-pr3 broadcast, reduce_scatter, barrier, all_to_all, all_to_all_v

End state: the pass file drops to 626 lines — dispatch table, mutator and Pass() factory,
plus the elementwise tile.sin/tile.cos and MX tile.tquant_mx rules, which share no code
with the collectives and are out of scope here (see #2632, which documents all three domains).

Verification

Sim Docker (pypto3-hw-native-sys:sim), mounted worktree + pip install --no-build-isolation,
build parallelism capped:

pytest tests/ut/ir/transforms/ tests/ut/ir/test_distributed_ops.py \
       tests/ut/codegen/distributed/test_host_orch_distributed.py
  • 4319 passed, unchanged from before the split — the no-behaviour-change signal
  • clang-tidy (tests/lint/clang_tidy.py --diff-base origin/main) clean
  • pre-commit clean

Each phase of the stack was verified independently, not just the tip.

Not covered here

Distributed ST on NPU (tests/st/distributed/, sim P=2/P=4) has not been run — that needs
hardware via task-submit.

lower_composite_ops_pass.cpp has grown to 2845 lines and carries every
composite collective lowering rule in one translation unit, so each new
collective lands hundreds of lines in the same file and any edit risks
every other rule. Plan 70 pays this down by splitting per-collective
translation units without touching pass order (the reorder alternative
was rejected in hw-native-sys#1850).

This is phase 1: move the shared LoweringBuilder scratchpad, its
CommSetup result struct and the MakeNegation helper into a
lower_composite/ module so the per-collective rules extracted next have
a shared home to depend on. Pure code motion — the rules still resolve
LoweringBuilder and CommSetup through using declarations, so no call
site changes and no IR output changes.

Mirrors public issue hw-native-sys#2632, which proposes the same split.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b9bded51-d679-403f-8ef4-e7b55224622f

📥 Commits

Reviewing files that changed from the base of the PR and between 74e0e81 and 537d320.

📒 Files selected for processing (4)
  • CMakeLists.txt
  • src/ir/transforms/lower_composite/lower_composite_builder.cpp
  • src/ir/transforms/lower_composite/lower_composite_builder.h
  • src/ir/transforms/lower_composite_ops_pass.cpp

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The PR extracts LoweringBuilder and CommSetup from the composite lowering pass into shared header and source files. The pass uses the shared types, and CMake includes the new source in the native build.

Changes

Composite lowering builder

Layer / File(s) Summary
Builder contract and implementation
src/ir/transforms/lower_composite/lower_composite_builder.h, src/ir/transforms/lower_composite/lower_composite_builder.cpp
Adds CommSetup and LoweringBuilder. The builder emits primitive operations, communication operations, barriers, epilogue resets, nested control flow, temporary bindings, and buffered statements.
Pass integration and native build
src/ir/transforms/lower_composite_ops_pass.cpp, CMakeLists.txt
Replaces the pass-local builder with the shared types and adds the new implementation file to PYPTO_SOURCES.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant lower_composite_ops_pass
  participant LoweringBuilder
  participant IR_statement_buffer
  lower_composite_ops_pass->>LoweringBuilder: construct lowering operations
  LoweringBuilder->>IR_statement_buffer: append IR statements
  lower_composite_ops_pass->>LoweringBuilder: TakeStmts()
  LoweringBuilder-->>lower_composite_ops_pass: return buffered statements
Loading

Merge Risk: ⚪ Minimal · up to 537d3

The extraction is integrated into the native build with no supported behavior regression identified, so it is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 3 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main refactoring: extracting LoweringBuilder from lower_composite_ops_pass as phase 1 of plan 70.
Description check ✅ Passed The description directly explains the code-motion refactoring, its scope, verification results, and excluded tests. It is fully related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit builds with careful paws
New tiles arise from shared laws
Signals wait and barriers align
Temp names hop in a tidy line
The lowering path now blooms anew

Comment @coderabbitai help to get the list of available commands.

@georgebisbas
georgebisbas marked this pull request as ready for review September 16, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant