Skip to content

fix(instrument): remove 5 of 6 roundtrip suppressions and fix root causes#971

Merged
Hzfengsy merged 1 commit intohw-native-sys:mainfrom
huxinyuan1215:fix/929-remove-roundtrip-suppressions
Apr 11, 2026
Merged

fix(instrument): remove 5 of 6 roundtrip suppressions and fix root causes#971
Hzfengsy merged 1 commit intohw-native-sys:mainfrom
huxinyuan1215:fix/929-remove-roundtrip-suppressions

Conversation

@huxinyuan1215
Copy link
Copy Markdown
Contributor

Summary

Closes #929

Remove 5 of 6 error message suppressions from RoundtripInstrument structural equality check, fixing underlying issues at their source instead of papering over them.

Suppressions removed (5)

Additional fixes

  • Fixed for-loop vars/bounds in test_verify_ssa_pass.py to use INDEX dtype (matching DSL pl.range() behavior) instead of INT64
  • Extracted _load_call() helper in test_legalize_pto_buffer_reuse.py to reduce duplication

Suppression kept (1)

Test plan

  • All 3416 unit tests pass (0 failures)
  • Verified Variable pointer mismatch suppression is still needed (test_dynamic_shape fails without it)
  • Verified removed suppressions are no longer triggered by any test

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR narrows roundtrip structural-equality suppression to only the "Variable pointer mismatch" case and updates printer/test code to stop relying on the suppressed special-cases: it removes tile.load printer special-casing, centralizes tile.load test construction, normalizes op IDs, adds call result types in SSA tests, and standardizes loop index types in SSA verification tests.

Changes

Cohort / File(s) Summary
Structural Equality Simplification
python/pypto/ir/instruments.py
Removed multiple specific exception-suppression branches in the roundtrip _after_pass handler; retained a single suppression for "Variable pointer mismatch" with expanded documentation and a TODO for parser Var reuse.
Printer Refactoring
src/ir/transforms/python_printer.cpp
Deleted the tile.load-specific printing branch that previously expanded 3-arg calls into a full form with valid_shapes, target_memory, and transpose; tile.load now prints via the generic call path.
Test Helper for tile.load
tests/ut/ir/transforms/test_legalize_pto_buffer_reuse.py
Added _load_call(source, offsets, shapes, tile_type) helper and replaced inline tile.load call constructions with it, unifying the use of valid_shapes, target_memory, and transpose.
Operation ID Normalization in Tests
tests/ut/ir/transforms/test_normalize_stmt_structure_pass.py
Replaced tensor.addtensor.adds and tensor.multensor.muls across before/expected IR test constructions to keep op identifiers consistent.
SSA Test Call Result Types
tests/ut/ir/transforms/test_convert_to_ssa_pass.py
Updated ir.Call constructions for tensor.add to include the tensor_type result argument where previously omitted.
Loop Index Type Standardization in Tests
tests/ut/ir/transforms/test_verify_ssa_pass.py
Changed ForStmt loop variables and constant bounds from DataType.INT64 to DataType.INDEX across multiple SSA verification tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • lyfne123
  • Hzfengsy

Poem

🐰 I nosed through IR, unearthing the trace,

I nudged silent skips — now bugs show their face.
No more quiet returns hiding the fight,
The parser will mend, and the prints now are right.
Hop, hop, review — let correctness take flight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing 5 of 6 roundtrip suppression branches from the instrument logic and addressing their root causes.
Description check ✅ Passed The description clearly explains what suppressions were removed, why, and what fixes were applied instead, demonstrating strong alignment with the changeset.
Linked Issues check ✅ Passed The PR fully addresses issue #929 by removing 5 of 6 suppressions and fixing their root causes; the kept suppression and its tracking issue #970 are documented.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing the linked issue #929: instrument logic refinement, test fixes for removed suppressions, and helper function extraction.
Docstring Coverage ✅ Passed Docstring coverage is 82.14% which is sufficient. The required threshold is 80.00%.

✏️ 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.

❤️ Share

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

@huxinyuan1215 huxinyuan1215 force-pushed the fix/929-remove-roundtrip-suppressions branch from caa0721 to 876e7d5 Compare April 11, 2026 13:31
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the RoundtripInstrument by removing several legacy error suppressions for IR asymmetries that have been resolved, such as UnknownType and operator name mismatches. It also simplifies the Python printer by removing special-case logic for tile.load and updates unit tests to use more accurate data types (e.g., DataType.INDEX for loop indices) and standardized helper functions. One review comment suggests updating a TODO issue reference from #929 to #970 to correctly track the remaining parser limitation regarding variable pointer reuse.

Comment on lines +104 to +105
# TODO(#929): fix the parser to reuse same-named dynamic-shape Var
# objects across param types, return types, and body — then remove.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The TODO references issue #929, but the pull request description indicates that #929 is closed by this PR and the root cause for the remaining suppression is now tracked in #970. The issue number should be updated to #970 for consistency and accurate tracking.

Suggested change
# TODO(#929): fix the parser to reuse same-named dynamic-shape Var
# objects across param types, return types, and body — then remove.
# TODO(#970): fix the parser to reuse same-named dynamic-shape Var
# objects across param types, return types, and body — then remove.

@huxinyuan1215 huxinyuan1215 force-pushed the fix/929-remove-roundtrip-suppressions branch 3 times, most recently from a0405cd to 60aee1c Compare April 11, 2026 14:02
…uses (hw-native-sys#929)

Remove error message suppressions from RoundtripInstrument that were
hiding real issues in tests, the printer, and type inference. Fix the
underlying problems at their source instead:

- Remove UnknownType suppression: fix tests to pass correct result type
  to ir.Call instead of relying on UnknownType default
- Remove tensor.add/adds suppression: fix tests to use tensor.adds for
  scalar operands (matching DSL dispatch behavior)
- Remove tile.load 3→4 args suppression: fix tests to construct 4-arg
  tile.load with valid_shapes and kwargs; remove printer special case
  that padded 3-arg to 4-arg
- Remove TileType tile_view suppression: fix InheritTileViewLayout to
  also propagate slayout (was only copying blayout and pad)
- Remove SplitMode suppression: already fixed upstream (hw-native-sys#964)
- Fix for-loop vars/bounds in tests to use INDEX dtype (matching DSL
  pl.range() behavior) instead of INT64

Keep only Variable pointer mismatch suppression — the parser creates
separate Var objects for same-named dynamic-shape parameters across
param types, return types, and body. Tracked in hw-native-sys#970.
@huxinyuan1215 huxinyuan1215 force-pushed the fix/929-remove-roundtrip-suppressions branch from 60aee1c to 67f8286 Compare April 11, 2026 14:19
@Hzfengsy Hzfengsy merged commit 53d9f93 into hw-native-sys:main Apr 11, 2026
8 checks passed
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.

[Code Health] Remove error message suppressions from RoundtripInstrument structural equality check

2 participants