Migrate to ktir-mlir-frontend 233d851: memory_space rename + tools_ktdp move - #208
Merged
Conversation
fabianlim
requested review from
kiszk,
lasch,
lchu6 and
nwang-ibm
as code owners
August 15, 2026 00:31
fabianlim
force-pushed
the
bump-ktir
branch
3 times, most recently
from
August 15, 2026 00:58
dcfde9b to
1fe8ddc
Compare
fabianlim
added a commit
to fabianlim/triton
that referenced
this pull request
Aug 15, 2026
The spyre-test extra pins fabianlim/ktir-cpu@bump-ktir, whose head moved to 1fe8ddc: the rank-0 regex changes were dropped (unrelated to this migration, and untested here — see torch-spyre/ktir-cpu#208), and its ktir-mlir-frontend pin was corrected ed069fb6 -> 233d851 so the bindings build picks up the cmake>=4.0 stubgen fix. Empty commit to re-run CI against that head. Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
…dp move
Bumps the `mlir-frontend` extra's pin 973d67e -> 233d851 and adapts to two
breaking changes in that range.
1. `a3d6bd1` replaced `#ktdp.spyre_memory_space<HBM|LX>` with
`#ktdp.memory_space<global|ct_local>`. This is not a pure rename — the enum
changed meaning, and the optional `core =` parameter became `ct_id =`.
Memory-space parsing is now centralized in `parser_utils.parse_memory_space`,
which accepts the new spelling and maps it onto the interpreter's existing
HBM/LX names so the execution model is unchanged:
global -> HBM
ct_local -> LX (ct_id -> lx_core_id)
Both frontends (regex and MLIR) go through it, so they cannot drift. The old
spelling is now explicitly rejected rather than silently defaulting to HBM;
see test_construct_memory_view_rejects_pre_rename_spelling.
2. `b4cb211` consolidated `tools_ktdp` into `mlir_ktdp/tools/` and `822079f`
inlined `ir_utils` into `tools/__init__.py`, so
`from tools_ktdp.ir_utils import ...` no longer resolves. That import sits
inside the `_HAS_MLIR` try/except, so the failure surfaced as the misleading
"mlir_ktdp is not installed" — the message is corrected too.
The pin is 233d851 rather than ed069fb6 (the tip of that PR range) because
ktir-mlir-frontend#59 sits on top and is required to build the bindings at all:
`05e7a0f` enabled MLIR_PYTHON_STUBGEN_ENABLED, which activated a latent
filename overflow (MLIR joins DEPENDS_TARGETS into a 283-char target name that
CMake 3.x writes into its file-API reply filename, past NAME_MAX). torch-spyre#59 pins
cmake>=4.0, which truncates the name. Repin to the squashed commit on main once
torch-spyre#59 lands.
Verified: 1322 passed, 10 skipped, 13 xfailed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
lasch
reviewed
Aug 17, 2026
Comment on lines
+513
to
+517
| # The dim portion is optional: a rank-0 tile is "index" alone, with no | ||
| # dims and hence no 'x' separator (emitted by Triton's LowerScalarLoad | ||
| # for a scalar read). "128" without an element type still fails, since | ||
| # group(2) is required. | ||
| type_match = re.match(r'^(?:(\d+(?:x\d+)*)x)?([a-zA-Z_]\w*)$', inner) |
Member
There was a problem hiding this comment.
should this rank-0 treatment become legal now. See offline discussion about dealing with rank-0.
Collaborator
Author
There was a problem hiding this comment.
sorry this was a runway commit. I was experimenting with rank-0 in a diff set of experiments and got contaminated in. thanks for finding it out. i have now removed it. This PR should be restricted to only renames of memory spaces and the removal of tools_ktdp.
Addresses review feedback on torch-spyre#208. Drop the unrelated rank-0 changes (lasch's review comment). Three hunks in `ktdp_ops.py` — the wrapped-form `parse_tensor_or_memref_type` call, the optional-dims `access_tile` regex, and the matching `access_shape` construction — were pre-existing uncommitted work for rank-0 / scalar-tensor support (Triton's LowerScalarLoad) that got swept into the previous commit. They are unrelated to the memory_space rename and belong in their own PR, so they are reverted to their pre-PR state here. `ktdp_ops.py` now differs from main only by the memory_space change. Test count drops 1326 -> 1322 because four rank-0 tests came along with those hunks; the two rank-0 tests that remain in tests/test_dialects_parse.py predate this branch and are untouched. State the RFC conflict explicitly. The previous wording said the new spelling "supersedes" RFC 0682 in a parenthetical, which reads as settled rather than as an open conflict needing a spec update. The rename is a semantic change, not just a spelling change — the enum went from naming *devices* (HBM/LX) to naming *visibility* (reachable by all compute tiles vs. private to one) — and `unspecified` was dropped with no replacement. Now recorded as: - docs/gap_analysis.md: new row 4a under a new⚠️ "spec conflict" legend entry, with the action item that RFC 0682 §MemorySpaceAttr be revised. Row 4's status is also corrected to ✅ (it used 🟢, absent from the legend). - CLAUDE.md and .claude/skills/ktir-dialect.md: flagged as a conflict, with the note that the dialect is the executable authority here because the RFC spelling no longer parses at all, and that reviews should not flag the new spelling as a deviation. Repin 233d851 -> ecfb9ed now that ktir-mlir-frontend#59 has merged; ecfb9ed is that merge commit and the current tip of main, so the temporary pin below the PR tip is no longer needed. The pinned LLVM hash is unchanged. The explicit `cmake>=4.0` install in ci.yml stays: CI installs with --no-build-isolation, which bypasses the `build-system.requires` where torch-spyre#59 declares that pin, so it must still be satisfied in the workflow. Its comment is reworded to say so rather than implying torch-spyre#59 is unmerged. Verified: 1322 passed, 10 skipped, 13 xfailed, with _HAS_MLIR True (the frontend suite ran rather than silently skipping). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
…dp move
Bumps the `mlir-frontend` extra's pin 973d67e -> ecfb9ed and adapts to two
breaking changes in that range.
1. `a3d6bd1` replaced `#ktdp.spyre_memory_space<HBM|LX>` with
`#ktdp.memory_space<global|ct_local>`. This is not a pure rename — the enum
changed meaning, and the optional `core =` parameter became `ct_id =`.
KTDP names memory spaces by visibility; the interpreter names the concrete
Spyre memories they map onto, so the two vocabularies are bridged by one
table in ir_types:
KTDP_MEMORY_SPACE_KINDS = {"global": "HBM", "ct_local": "LX"}
Both frontends (regex in dialects/ktdp_ops.py, MLIR in
mlir_frontend/parser.py) and MemRef's own validator read that single table,
so they cannot drift. The execution model is unchanged. The old spelling is
now rejected rather than silently defaulting to HBM; see
test_construct_memory_view_rejects_pre_rename_spelling.
2. `b4cb211` consolidated `tools_ktdp` into `mlir_ktdp/tools/` and `822079f`
inlined `ir_utils` into `tools/__init__.py`, so
`from tools_ktdp.ir_utils import ...` no longer resolves. That import sits
inside the `_HAS_MLIR` try/except, so the failure surfaced as the misleading
"mlir_ktdp is not installed" — the message is corrected too.
CI also needs `cmake>=4.0` at the pre-install step. ktir-mlir-frontend#59
(merged as ecfb9ed) declares that in build-system.requires, but this workflow
installs with --no-build-isolation, which bypasses build-system.requires
entirely — so the pin has to be satisfied here as well.
Verified: 1302 passed, 10 skipped, 13 xfailed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
fabianlim
enabled auto-merge (squash)
August 18, 2026 00:35
Collaborator
Author
|
@lasch ready for another look. I removed that stray rank-0 change, and also I refactored abit more of the code, as I felt the earlier version was introducing unncessary helpers that made it look over-complicated |
Collaborator
|
LGTM. |
nwang-ibm
previously approved these changes
Aug 18, 2026
The RoPE kernel (torch-spyre#194) landed on main after this branch renamed `#ktdp.spyre_memory_space<HBM>` to `#ktdp.memory_space<global>`, so the merge of main brought in four occurrences of the old spelling. It no longer parses under the bumped ktir-mlir-frontend pin, which is why the frontend job failed on the merge commit while the regex-parser job passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps the
mlir-frontendextra'sktir-mlir-frontendpin973d67e→233d851and adapts to two breaking changes in that range.Needed by torch-spyre/triton#108, which bumps the same submodule. Until this lands, that PR's numerical tests fail — every
test_numerical[*]dies in the MLIR parse worker.1.
spyre_memory_space→memory_space(a3d6bd1)Not a pure rename — the enum changed meaning and the optional parameter was renamed:
Memory-space parsing is now centralized in
parser_utils.parse_memory_space, which accepts the new spelling and maps it onto the interpreter's existing internal names, so the execution model is unchanged:Both frontends (regex and MLIR) route through that one helper, so they can't drift apart.
The old spelling is now explicitly rejected rather than silently defaulting to HBM — see
test_construct_memory_view_rejects_pre_rename_spelling. Worth a look during review: silently accepting the pre-rename form would have been the more dangerous failure, since it would place data in the wrong memory space rather than erroring.2.
tools_ktdpmoved intomlir_ktdp.toolsb4cb211consolidatedtools_ktdpintomlir_ktdp/tools/, and822079finlinedir_utilsintotools/__init__.py, sofrom tools_ktdp.ir_utils import ...no longer resolves.That import lives inside the
_HAS_MLIRtry/except ImportError, so the failure surfaced as the misleading "mlir_ktdp / tools_ktdp not installed" — withmlir_ktdpin fact installed and working. This cost real debugging time on the triton side, so the message is corrected to name only what it actually checks.Scope
51 files, +422/−223 — mostly mechanical (163 attribute substitutions across
examples/andtests/). The substantive changes are confined to 5 files underktir_cpu/, plus the CI workflow below:parser_utils.pyparse_memory_space(+72)mlir_frontend/parser.pydialects/ktdp_ops.py,ir_types.py,ops/memory_ops.pypyproject.toml/uv.lockcarry the pin bump itself.Also: CI needed
cmake>=4.0Installing the new frontend pin fails in CI with
OSError: [Errno 36] File name too long, and the fix has to live here rather than upstream.torch-spyre/ktir-mlir-frontend#59 addresses the root cause — MLIR's
mlir_generate_type_stubs()joinsDEPENDS_TARGETSinto a 283-char target name, and CMake 3.x writes that whole name into its file-API reply filename, pastNAME_MAX(255). #59 pinscmake>=4.0inbuild-system.requires, which truncates the name and relies on the trailing hash.But this workflow installs with
--no-build-isolation, which bypassesbuild-system.requiresentirely, so that pin never applies. The pre-install step supplied onlyscikit-build-coreandnanobind, leaving the build to use the runner's CMake 3.x. Addingcmake>=4.0/ninja>=1.11there satisfies it directly.This is why the pin is
233d851(the #59 branch head) rather thaned069fb6— and why it alone was not sufficient.Testing
1322 passed, 10 skipped, 13 xfailed locally; CI green (1191 passed via the MLIR parser, 131 via the regex parser).
Cross-checked against torch-spyre/triton#108 with this branch installed: the 106 failures there drop to 0 (1146 passed, 47 skipped).