Skip to content

[FIRRTL] Fix mask repeat order in FlattenMemory#10106

Open
Waryc wants to merge 1 commit intollvm:mainfrom
Waryc:fix/flatten-memory-mask-order
Open

[FIRRTL] Fix mask repeat order in FlattenMemory#10106
Waryc wants to merge 1 commit intollvm:mainfrom
Waryc:fix/flatten-memory-mask-order

Conversation

@Waryc
Copy link
Copy Markdown

@Waryc Waryc commented Apr 3, 2026

Fixes #10105

Summary

Fix incorrect mask-bit repetition ordering in FlattenMemory when expanding aggregate memory masks.

The previous logic indexed repetition counts by FIRRTL bit index directly, while maskWidths is ordered MSB-to-LSB. This mismatch could assign repetition counts to the wrong mask bits for mixed-width aggregates.

Changes

  • Use reverse index mapping when reading repetition count from maskWidths in FlattenMemory.cpp.
  • Add and adjust flatten-memory.mlir checks to match corrected ordering.
  • Add a MaskRepeatOrder regression case for a compact mixed-width aggregate.

Validation

  • Built tools with Ninja: firtool, circt-opt, FileCheck.
  • Verified targeted test via circt-opt + FileCheck on test/Dialect/FIRRTL/flatten-memory.mlir.
  • Ran llvm-lit -sv test/Dialect/FIRRTL/flatten-memory.mlir.
  • Re-ran firtool on m.fir (in the related issue) and confirmed corrected mask concatenation ordering in both FIR and Verilog outputs.

Assisted-by

  • GPT-5.3-Codex: Created minimal reproducible examples, assisted in reading and guiding modifications to CIRCT code, contributed to regression tests, and drafted the bug report along with this pull request.
  • DeepSeek-R1: Handled partial translation tasks.

My role: Reviewed and assumed responsibility for all AI-generated content, with minor textual refinements.

Use mask width lookup aligned with FIRRTL bit index order when expanding aggregate masks in FlattenMemory.\n\nAlso update and extend flatten-memory regression checks, including a MaskRepeatOrder case and adjusted expectations for corrected mask mapping.
Copilot AI review requested due to automatic review settings April 3, 2026 08:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes incorrect ordering when repeating/expanding aggregate memory mask bits during FlattenMemory, ensuring repetition counts align with FIRRTL bit ordering (MSB→LSB) rather than raw bit indices.

Changes:

  • Adjust mask-bit repetition to use a reverse index mapping when looking up repetition counts.
  • Update existing FileCheck patterns to reflect the corrected concatenation order.
  • Add a regression test covering a compact mixed-width aggregate mask expansion case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/Dialect/FIRRTL/flatten-memory.mlir Updates existing checks for corrected mask concatenation order and adds a new regression module @MaskRepeatOrder.
lib/Dialect/FIRRTL/Transforms/FlattenMemory.cpp Fixes repetition-count lookup by reversing the index used to access maskWidths during mask expansion.

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.

[FIRRTL] Suspected incorrect memory mask lowering for aggregate partial write with dynamic subaccess

2 participants