fix(printer): print atomic kwarg as pl.AtomicType enum, not raw int#1538
Conversation
The Python printer emitted `pl.tile.store(..., atomic=1)` for stores carrying the atomic-add combine mode, but the DSL signature is `atomic: AtomicType`. Static checkers (Pyright) flagged the printed source as `Literal[1]` incompatible with `AtomicType`. Restore the enum form in the printer, mirroring the existing `set_pipe` / `wait_pipe` -> `pl.PipeType.<Name>` handling. Storage on `Call::kwargs_` stays `int` (consistent with how the binding layer casts all `AtomicType` / `PipeType` / etc. enums to int). Round-trip preserved: `pl.AtomicType` is already exposed in `pypto.language`, so the parser resolves `pl.AtomicType.Add` via normal Python attribute lookup. Covers `tile.store`, `tensor.assemble`, and `pld.tensor.put`, which all share the printer path. Also fix two pre-existing misc-include-cleaner violations in `src/ir/op/distributed/put.cpp` surfaced by adding `<string>` / `pypto/core/error.h` to `comm.h`: missing direct includes for `<cstddef>` (size_t) and `pypto/core/logging.h` (CHECK macro).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds round-trippable printing of the ChangesAtomicType Enum Printing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes the Python printer emitting atomic=1 (raw int) for ops carrying the atomic kwarg, which violated the DSL's atomic: AtomicType signature and tripped Pyright. The printer now emits pl.AtomicType.<Name>, mirroring the existing PipeType handling.
Changes:
- Add
AtomicTypeToStringhelper ininclude/pypto/ir/comm.hand dispatch on theatomickwarg inIRPythonPrinter::VisitExpr_for Calls. - Fix incidental
misc-include-cleanerviolations insrc/ir/op/distributed/put.cppexposed by adding<string>/pypto/core/error.htocomm.h. - New unit test pinning both
AddandNone_enum-form printing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| include/pypto/ir/comm.h | Adds AtomicTypeToString helper; pulls in <string> and pypto/core/error.h. |
| src/ir/transforms/python_printer.cpp | Prints atomic kwarg as pl.AtomicType.<Name> instead of raw int. |
| src/ir/op/distributed/put.cpp | Adds direct includes for <cstddef> and pypto/core/logging.h. |
| tests/ut/ir/printing/test_python_printer.py | New test pinning the enum-form printing for Add and None_. |
There was a problem hiding this comment.
Code Review
This pull request adds support for printing the atomic keyword argument using its Python enum form (pl.AtomicType.<Name>) instead of a raw integer in the Python printer, ensuring type correctness and round-trip compatibility. It also includes corresponding unit tests. The review feedback suggests keeping the core C++ IR header comm.h clean by localizing the Python-specific string conversion logic for AtomicType directly inside python_printer.cpp, which aligns with existing patterns for other enums and avoids header bloat.
| #include <string> | ||
|
|
||
| #include "pypto/core/error.h" | ||
|
|
There was a problem hiding this comment.
Separation of Concerns & Header Bloat
Including <string> and "pypto/core/error.h" in a core IR header like comm.h introduces unnecessary compilation coupling and increases header bloat.
Furthermore, defining Python-specific printing helpers (such as AtomicTypeToString which handles the Python-specific "None_" suffix) inside a core C++ IR header couples the core IR definitions with Python-specific printer details.
We should keep comm.h clean and handle this string conversion locally within python_printer.cpp, matching how other printer-specific conversions (like CastModeToString and SplitModeToPythonString) are handled.
|
|
||
| // Convert AtomicType to the matching Python enum member name. The Python | ||
| // member is `None_` (trailing underscore) because `None` is a reserved word — | ||
| // keep this in sync with the `nb::enum_<AtomicType>` binding in | ||
| // `python/bindings/modules/ir.cpp`. | ||
| inline std::string AtomicTypeToString(AtomicType atomic) { | ||
| switch (atomic) { | ||
| case AtomicType::kNone: | ||
| return "None_"; | ||
| case AtomicType::kAdd: | ||
| return "Add"; | ||
| default: | ||
| throw pypto::TypeError("Unknown AtomicType: " + std::to_string(static_cast<int>(atomic))); | ||
| } | ||
| } | ||
|
|
| } else if (key == "atomic") { | ||
| // Stored as int (the DSL casts AtomicType -> int before stashing on | ||
| // kwargs_; nb::isinstance<AtomicType> in bindings does the same). The | ||
| // public DSL signature is `atomic: AtomicType`, so restore the enum | ||
| // form on print to keep the output type-correct for static checkers | ||
| // and round-trippable through the parser (pl.AtomicType is exposed). | ||
| stream_ << prefix_ << ".AtomicType." << AtomicTypeToString(static_cast<AtomicType>(int_val)); |
There was a problem hiding this comment.
Localize Python-Specific Printing Logic
Instead of calling a helper from the core comm.h header, we can inline the AtomicType string conversion directly here. This keeps all Python-specific printing logic (including the "None_" suffix workaround) local to the printer, matching the established patterns for other enums in this file. For the default case in the enum switch, use INTERNAL_UNREACHABLE to align with project standards.
} else if (key == "atomic") {
// Stored as int (the DSL casts AtomicType -> int before stashing on
// kwargs_; nb::isinstance<AtomicType> in bindings does the same). The
// public DSL signature is atomic: AtomicType, so restore the enum
// form on print to keep the output type-correct for static checkers
// and round-trippable through the parser (pl.AtomicType is exposed).
std::string atomic_str;
switch (static_cast<AtomicType>(int_val)) {
case AtomicType::kNone:
atomic_str = "None_";
break;
case AtomicType::kAdd:
atomic_str = "Add";
break;
default:
INTERNAL_UNREACHABLE;
}
stream_ << prefix_ << ".AtomicType." << atomic_str;References
- For partial switch statements over enums, use a default case with INTERNAL_UNREACHABLE for clarity.
Summary
pl.tile.store(..., atomic=1)for stores carrying the atomic-add combine mode, but the DSL signature isatomic: AtomicType. Pyright flagged the printed source asLiteral[1]incompatible withAtomicType.pl.AtomicType.Add/pl.AtomicType.None_), mirroring the existingset_pipe/wait_pipe→pl.PipeType.<Name>handling.Call::kwargs_storage staysint(consistent with the binding layer that casts all enums → int).pl.AtomicTypeis already exposed inpypto.language, so the parser resolvespl.AtomicType.Addvia normal Python attribute lookup. Coverstile.store,tensor.assemble, andpld.tensor.put(all share the printer path).misc-include-cleanerviolations insrc/ir/op/distributed/put.cpp(missing direct includes for<cstddef>/pypto/core/logging.h) that were surfaced by adding<string>/pypto/core/error.htocomm.h.Before / After
tile.storewith atomic-add combine mode in a dumped IR file:Test plan
tests/ut/ir/printing/test_python_printer.py::test_python_print_atomic_kwarg_uses_enum_form(new) — pins bothAddandNone_print formstests/ut/ir/parser/test_put_op.py::test_put_round_trips_through_printer_and_parser— existing round-trip via parse → printtests/ut/jit/test_split_k.py::test_split_k_matmul_*— end-to-end split-K with atomic-add codegentests/ut/ir/,tests/ut/language/,tests/ut/codegen/test_pto_codegen_ops.py— 4347 passed, 28 skippedclang-tidyclean