feat: deprecate pl.Tensor[..., pl.DN] + strict TensorViewCanonical (RFC #1300 §2.4 + supplementary 1)#1347
Merged
Conversation
…pl.DN] (RFC hw-native-sys#1300 supplementary 1 + 2) Implements both RFC hw-native-sys#1300 supplementary proposals (Q4 + Q5) as ``DeprecationWarning``s at the user-facing surface. End-to-end behaviour is unchanged — existing code continues to compile, but each deprecated form now emits a warning suggesting the canonical replacement. **Q5 — ``pl.load(..., transpose=True)`` (DSL deprecation):** - ``pl.load`` in ``python/pypto/language/op/tile_ops.py`` emits a ``DeprecationWarning`` when ``transpose=True`` is passed. The migration hint points to ``pl.transpose(tensor, -2, -1) + pl.load(...)`` — see RFC §4.2 canonical pair: ``pl.transpose`` is a pure metadata reinterpret, and a DN-tagged ``tile.load`` produces the same TileType the old ``transpose=True`` swap produced. - Rationale (RFC supplementary 2): mixing a view-reinterpret into a memory-copy op breaks the orthogonality of ``pl.slice`` / ``pl.reshape`` / ``pl.transpose``. The new pattern is also more reusable — one ``b_t = pl.transpose(b, -2, -1)`` can feed multiple K-tile loads. **Q4 — ``pl.Tensor[..., pl.DN]`` (DSL deprecation):** - ``TypeResolver`` in ``python/pypto/language/parser/type_resolver.py`` emits a ``DeprecationWarning`` when the layout-only DN shorthand is parsed in a tensor type annotation (both 3-arg and 4-arg forms). The migration hint lists the three canonical replacement patterns: - source tensor shape, no layout marker; - derive DN at use site via ``pl.transpose(x, -2, -1)``; - inherit DN through slice/reshape from a DN-producing op. - Rationale (RFC supplementary 1): the layout-only shorthand forces users to mentally hold two coordinate systems at once (the IR-logical post-view shape and the runtime row-major shape) — exactly the ambiguity RFC hw-native-sys#1300 aims to eliminate. ``pl.TensorView(stride=[...], layout=pl.TensorLayout.DN)`` is still accepted (explicit stride form for canonical pretty-print round-trip). **Migration:** - ``examples/models/04_paged_attention.py`` and ``06_paged_attention_dynamic.py`` are migrated to demonstrate the new pattern. Other examples and tests that still use the deprecated forms will surface ``DeprecationWarning``s during execution; they can be migrated incrementally before the eventual hard removal. - ``docs/en/user/01-language_guide.md`` and ``docs/zh-cn/user/01-language_guide.md`` rewrite the Tensor Layouts section and add a new "Matmul B^T via Transposed View" pattern. Unit tests: cmake build clean, ``pytest tests/ut/ -n auto`` → **4631 passed / 41 skipped / 0 failed**. (Existing tests continue to exercise the deprecated forms — pytest doesn't fail on ``DeprecationWarning`` by default; we observe ~140 new warnings from internal test fixtures that intentionally use the legacy patterns.) Related: closes Q4 + Q5 from the RFC hw-native-sys#1300 supplementary proposal.
📝 WalkthroughWalkthroughThis PR deprecates two transpose-related shortcuts in the pypto language: the ChangesTranspose and DN layout deprecation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en/user/01-language_guide.md`:
- Around line 78-80: The doc text now states pl.NZ is tile-only but the earlier
example still uses pl.NZ as a TensorType annotation (pl.Tensor[[64, 128],
pl.FP16, pl.NZ]); update that example to match the rule by removing pl.NZ from
the TensorType and instead showing NZ used in a tile context (e.g., use
pl.Tensor[[64,128], pl.FP16] and show pl.Tile[..., pl.NZ] for the NZ usage) so
examples and explanation (pl.NZ, pl.Tile, pl.Tensor) are consistent.
In `@docs/zh-cn/user/01-language_guide.md`:
- Around line 73-74: The NZ usage in earlier example is inconsistent: NZ is
tile-only and should not appear as a TensorType annotation; update the earlier
example that uses pl.Tensor[..., pl.NZ] (the one showing pl.Tensor[[64, 128],
pl.FP16, pl.NZ]) to remove pl.NZ from the TensorType and instead show NZ in a
pl.Tile expression (e.g., demonstrate pl.Tile[..., pl.NZ] or use pl.Tensor[[64,
128], pl.FP16] with a separate pl.Tile[..., pl.NZ] note), ensuring all examples
consistently treat pl.NZ as tile-only and keep references to pl.NZ, pl.Tile, and
pl.Tensor accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aaf859bf-e9ac-4c96-8ce0-4fc386f32b6a
📒 Files selected for processing (6)
docs/en/user/01-language_guide.mddocs/zh-cn/user/01-language_guide.mdexamples/models/04_paged_attention.pyexamples/models/06_paged_attention_dynamic.pypython/pypto/language/op/tile_ops.pypython/pypto/language/parser/type_resolver.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request deprecates the explicit pl.DN layout shorthand in tensor type annotations and the transpose=True parameter in pl.load, favoring the use of pl.transpose to derive transposed views. These changes align with RFC #1300 to ensure orthogonality and reduce coordinate system ambiguity. Documentation and examples have been updated accordingly, and deprecation warnings are now emitted during parsing and at runtime. Feedback suggests refining the deprecation warning message for distributed tensors to avoid potential namespace confusion.
| if layout != ir.TensorLayout.DN: | ||
| return | ||
| warnings.warn( | ||
| f"pl.{type_name}[..., pl.DN] is deprecated (RFC #1300 supplementary 1). " |
Contributor
There was a problem hiding this comment.
The warning message uses pl.{type_name} as a prefix. While this is correct for Tensor, DistributedTensor is typically imported from the pypto.language.distributed namespace (often aliased as pld). If a user uses pld.DistributedTensor[..., pl.DN], the warning pl.DistributedTensor[...] might be slightly confusing. Consider making the prefix more generic or detecting the actual namespace if possible, though pl. is a reasonable default for the project.
…recation guidance System tests (a2a3) on PR hw-native-sys#1347 failed because the migrated examples (``04_paged_attention.py`` / ``06_paged_attention_dynamic.py``) used ``pl.transpose(tensor) + pl.load(transposed)``, but this composition is not currently supported end-to-end: ``ConvertTensorToTileOps`` registers ``tensor.transpose → tile.transpose`` (via ``RegisterSimple``), so the ``pl.transpose`` result becomes a TileType, and the downstream ``pl.load`` type check rejects it ("requires first argument to be a TensorType, but got TileType" at memory.cpp:98). This commit: - Reverts the two example migrations back to the deprecated ``pl.load(..., transpose=True)`` form so the system tests pass again. - Softens the ``DeprecationWarning`` text in ``tile_ops.py:load`` to be honest about the migration status — it explains that the intended end state (tensor-level view + plain ``pl.load``) is not yet composable in the pipeline, asks callers to avoid introducing new uses, and clarifies that no rewrite is required yet. - Rewrites the "Matmul B^T via Transposed View" section in ``docs/{en,zh-cn}/user/01-language_guide.md`` to surface the same migration-status caveat, with the deprecated form labeled as "keep using" until the lowering fix lands. Q4 deprecation (``pl.Tensor[..., pl.DN]``) is unaffected — that one has a working migration today via the three patterns listed in the warning (source-shape, ``pl.transpose`` at use site for non-load consumers, ``pl.slice`` from a DN-producing op). Follow-up: a separate PR will update ``ConvertTensorToTileOps`` so ``tensor.transpose`` whose consumer is ``tile.load`` stays as a tensor-level view (and the existing ``LowerTransposeLoadParamLayout`` pass picks up the slack). After that, the ``pl.transpose + pl.load`` migration becomes usable and we can re-migrate examples + tighten the deprecation message. Unit tests: cmake build clean, ``pytest tests/ut/ -n auto`` → 4631 passed / 41 skipped / 0 failed.
…ToTileOps lowering is intentional Per maintainer feedback: ``ConvertTensorToTileOps`` registering ``tensor.transpose → tile.transpose`` (via ``RegisterSimple``) is the correct lowering for the existing pipeline; the RFC hw-native-sys#1300 supplementary 2 ``pl.transpose + pl.load`` migration path was the wrong shape for it (and broke system tests on PR hw-native-sys#1347's first attempt). Deprecating ``pl.load(..., transpose=True)`` without a working migration target is misleading, so this commit drops Q5 entirely from this PR. Changes: - ``python/pypto/language/op/tile_ops.py``: remove the ``DeprecationWarning`` in ``load(...)`` and revert the docstring back to its original form (no migration note for ``transpose=True``). - ``docs/{en,zh-cn}/user/01-language_guide.md``: delete the "Pattern: Matmul B^T via Transposed View" section that was promoting the unsupported migration; also tone down the Tensor Layouts section so it no longer recommends ``pl.transpose(t, -2, -1)`` as a migration target for the deprecated ``pl.Tensor[..., pl.DN]`` shorthand. The recommended replacement now is simply: drop the layout marker, write the runtime shape, and continue using ``pl.load(transpose=True)`` for matmul B^T. Q4 (``pl.Tensor[..., pl.DN]`` deprecation) is unchanged — that path has a working migration today via "drop the layout marker, use runtime shape" plus the existing ``transpose=True`` mechanism. Validation: ``cmake --build`` clean; ``pytest tests/ut/ -n auto`` → 4631 passed / 41 skipped / 0 failed. Test-suite DeprecationWarning count dropped from 650 → 532, matching the removal of the per-call Q5 warning.
…ative-sys#1300 §2.4 codegen-entry contract) Closes RFC hw-native-sys#1300 §6 roadmap item: the registry-default verifier for ``IRProperty::TensorViewCanonical`` now enforces explicit stride (``require_materialized=true``). Because the verifier auto-fires after ``MaterializeTensorStrides`` (which declares it as a produced property), the "codegen entry has explicit stride" contract becomes a hard invariant instead of a convention. What this catches: - ``view.has_value() && stride.empty()`` reaching the post-MaterializeTensorStrides boundary. That state was previously tolerated by the weak-mode default; callers that constructed empty-stride views and bypassed the materialization pass would silently slip through. What stays accepted: - Bare ``TensorType`` (``!view.has_value()``) — implicit ND-packed is canonical by construction. - Explicit canonical strided views (``stride[-1]==1`` for ND, ``stride[-2]==1`` for DN, outer-dim ``stride[k] >= stride[k+1]*shape[k+1]``) — the RFC §2.2 strided-family forms. Public API: ``passes.verify_tensor_view_canonical(program, require_materialized=)`` still accepts both modes; pass ``False`` for the parse-time / early-pass window before materialization runs. Files changed: - ``src/ir/verifier/property_verifier_registry.cpp``: flip ``require_materialized`` from ``false`` to ``true`` at the registry registration; rewrite the explanatory comment. - ``tests/ut/ir/transforms/test_verify_tensor_view_canonical.py``: the ``test_registry_returns_weak_verifier`` test asserted the old default (empty stride accepted). Replaced with ``test_registry_returns_strict_verifier`` (expects the rejection + matches the diagnostic message) and added ``test_registry_accepts_bare_tensor_type`` to lock in the bare-tensor carve-out. - ``docs/{en,zh-cn}/dev/passes/26-materialize_tensor_strides.md``: update the Produces / verifier-interaction notes to reflect the new default. Validation: cmake build clean; ``pytest tests/ut/ -n auto`` → 4632 passed / 41 skipped / 0 failed.
lyfne123
changed the title
Hzfengsy
approved these changes
May 12, 2026
Merged
4 tasks
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
2 participants
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.
Summary
Final DSL + verifier cleanup for RFC #1300. Two changes:
pl.Tensor[..., pl.DN]layout-only shorthand.TensorViewCanonicalverifier from weak-mode (default) to strict-mode, making the "codegen entry has explicit stride" contract a hard invariant.Q5 (
pl.load(transpose=True)deprecation) is not included — the proposed migration path (pl.transpose + pl.load) doesn't compose under the current pipeline (ConvertTensorToTileOpslowerstensor.transpose → tile.transpose, which is the intentional and correct behaviour), so deprecating without a working migration would be misleading. Reverted from an earlier iteration of this PR.Q4 —
pl.Tensor[..., pl.DN]deprecationWhat:
TypeResolveremits aDeprecationWarningwhen the layout-only DN shorthand is parsed in a tensor type annotation (both 3-arg and 4-arg forms).pl.TensorView(stride=[...], layout=pl.TensorLayout.DN)(explicit stride form) is still accepted for canonical pretty-print round-trip.Migration:
Why: the layout-only shorthand forces users to mentally hold two coordinate systems at once (the IR-logical post-view shape and the runtime row-major shape) — exactly the ambiguity RFC #1300 aims to eliminate.
Verifier — strict mode default
What: The registry-default verifier for
IRProperty::TensorViewCanonicalnow requires explicit stride (require_materialized=true). Since this verifier auto-fires afterMaterializeTensorStrides(which declares it as a produced property), the "codegen entry has explicit stride" contract becomes a hard invariant.Catches:
view.has_value() && stride.empty()reaching the post-MaterializeTensorStrides boundary. Previously tolerated; callers that constructed empty-stride views and bypassed the materialization pass would silently slip through.Stays accepted:
TensorType(!view.has_value()) — implicit ND-packed is canonical by construction.stride[-1]==1for ND,stride[-2]==1for DN, outer-dimstride[k] >= stride[k+1]*shape[k+1]).Escape hatch:
passes.verify_tensor_view_canonical(program, require_materialized=False)still callable for the parse-time / early-pass window before materialization runs.Files changed
python/pypto/language/parser/type_resolver.pyDeprecationWarningat 3-arg / 4-arg tensor type annotationssrc/ir/verifier/property_verifier_registry.cppIRProperty::TensorViewCanonicaltests/ut/ir/transforms/test_verify_tensor_view_canonical.pytest_registry_returns_weak_verifier→test_registry_returns_strict_verifier; newtest_registry_accepts_bare_tensor_typedocs/{en,zh-cn}/dev/passes/26-materialize_tensor_strides.mddocs/{en,zh-cn}/user/01-language_guide.mdValidation
cmake --build build --parallel— clean.pytest tests/ut/ -n auto --maxprocesses 8→ 4632 passed / 41 skipped / 0 failed.Test plan
DeprecationWarningfires whenpl.Tensor[..., pl.DN]is parsed.view.has_value() && stride.empty()is now rejected by the registry default.Related
pl.Tensor[..., pl.DN]deprecation).TensorViewCanonicalat codegen-entry.pl.load(transpose=True)deprecation) is not addressed here — the prerequisitetensor.transpose + tile.loadcomposition is a separate refactor; will be revisited in a follow-up if that path becomes desirable.