Skip to content

fix unknow pattern case#1162

Open
fangyangci wants to merge 1 commit into
mainfrom
fangyangci/fix_unknow_patterns
Open

fix unknow pattern case#1162
fangyangci wants to merge 1 commit into
mainfrom
fangyangci/fix_unknow_patterns

Conversation

@fangyangci

Copy link
Copy Markdown
Contributor

No description provided.

@fangyangci
fangyangci requested a review from a team as a code owner July 21, 2026 08:04

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the fix for the "unknown pattern" cases. The overall approach is sound: broaden the RTR transpose input-generator to cover 3-D outputs and merged_transpose_dim=5, and make the Gemm generator treat only scalar biases as value-constraints so vector/tensor biases stay shape-constrained (instead of emitting value-based rules that real models never match). I hand-checked the added transpose combinations — they're all element-count consistent, and the PixelShuffle case does resolve to merged_transpose_dim=5.

A few things to look at (details inline):

  • The Gemm-side behavioral change (_is_scalar_constraint_value / C_is_value_constraint) has no unit test — only the transpose generator got new tests.
  • pattern: Pattern lost its type annotation in GemmPatternInputGenerator; that looks unrelated to this fix and now contradicts the comment directly above it.
  • The new transpose tests assert the hard-coded fixture rather than the derivation → rule-matching path that actually produced "unknown".
  • End-to-end: the matcher loads precomputed rule tables (parquet/JSON pulled from a release, per rules/runtime_check_rules/README.md), so these generator changes won't change real analyses until the runtime-check rules are regenerated and re-released. Is that regeneration planned as part of landing this?

Note: I couldn't run the suite locally (ARM64 host vs. the AMD64/linux lockfile), so please rely on CI to confirm the new tests pass.

from .op_input_gen import InputShapeConstraint, InputValueConstraint


def _is_scalar_constraint_value(value: Any) -> bool:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scalar-vs-vector bias logic is the core behavioral change on the Gemm side, but there's no unit test for it in this PR (only the transpose generator got new tests). Please add coverage asserting C_is_value_constraint is True for a scalar bias and False for a 1-D/N-D vector constant and for None — that's exactly the distinction this fix hinges on, and it'd lock the regression in.


# Typed as the base Pattern so subclasses can set a different concrete pattern.
pattern: Pattern = ReshapeGemmReshapePattern()
pattern = ReshapeGemmReshapePattern()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops the explicit : Pattern annotation, but the comment right above still says "Typed as the base Pattern so subclasses can set a different concrete pattern." Without the annotation the attribute falls back to the base Pattern | None type, and this change looks unrelated to the unknown-pattern fix. Suggest restoring pattern: Pattern = ReshapeGemmReshapePattern() (or dropping the now-inaccurate comment).

c_value = item.get("C_value")
item["C_is_value_constraint"] = _is_scalar_constraint_value(c_value) and item[
"C_dim"
] == 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is and item["C_dim"] == 0 adding anything beyond _is_scalar_constraint_value(c_value)? A scalar / 0-d value already implies C_shape == ()C_dim == 0, so this second clause looks redundant. If it's intentional defensive coding for rows where value and shape disagree, a one-line comment would help; otherwise it can be dropped to keep the intent clear.

"data": InputShapeConstraint((1, 256, 256, 96)),
"transpose_shape": (1, 32, 8, 32, 8, 96),
"perm": (0, 1, 3, 2, 4, 5),
"output_shape": (1024, 64, 96),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good — this is the missing 3-D output coverage (and the later case adds merged_transpose_dim=5). One thing to confirm before this actually fixes anything for users: the runtime matcher loads precomputed rule tables (parquet/JSON from a release, per rules/runtime_check_rules/README.md), not the generators directly. So these new combinations only change classification once the runtime-check rules are regenerated and re-released. Is that regeneration planned as part of this change, or does it happen out-of-band?

class TestRTRPatternInputGeneratorCoverage:
"""Regression tests for RTR pattern input-generation coverage."""

def test_output_dim_coverage_includes_3d_and_4d(self) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests assert that the generator returns the combinations that were just hard-coded above — useful as a guard against accidental removal, but they don't exercise the path that actually produced the "unknown pattern" (property derivation → rule matching in runtime_checker_query). Consider adding a regression test that runs a representative previously-unknown subgraph through the matcher and asserts it's now classified, so the tests protect the behavior rather than the fixture.

@fangyangci
fangyangci force-pushed the fangyangci/fix_unknow_patterns branch from c81a272 to 92bc5a9 Compare July 22, 2026 13:25
@fangyangci
fangyangci force-pushed the fangyangci/fix_unknow_patterns branch from 9a94882 to 3bfb39d Compare July 22, 2026 13:28
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