Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL] FoldUnusedBits: Cast compressed data back to signed integer #7913

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

rwy7
Copy link
Contributor

@rwy7 rwy7 commented Nov 27, 2024

The FoldUnusedBits folder will, when it sees a memory with obviously unused bits (all readers are bitselect ops), will compress that memory to a smaller integer. The data written to the memory has to be compressed to just the relevant bits, which is done using a sequence of bit-select and bit-concat ops on the data. The result of these ops is unsigned. If the memory itself stores a signed integer, then we need to cast this data back to a signed value.

@rwy7 rwy7 force-pushed the fix-fold-reg-mem branch 2 times, most recently from 7f1ba9e to 1045203 Compare November 27, 2024 23:07
@rwy7 rwy7 added the FIRRTL Involving the `firrtl` dialect label Nov 27, 2024
test/Dialect/FIRRTL/simplify-mems.mlir Show resolved Hide resolved
// the memory.
if (type.isSigned())
catOfSlices =
rewriter.createOrFold<AsSIntPrimOp>(writeOp.getLoc(), catOfSlices);
Copy link
Member

Choose a reason for hiding this comment

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

Nice clean fix. 👍

@unlsycn
Copy link
Contributor

unlsycn commented Nov 28, 2024

This patch doesn't seem to fix chipsalliance/chisel#4536, please see this comment

@rwy7 rwy7 merged commit e2ebea5 into llvm:main Nov 28, 2024
4 checks passed
@rwy7 rwy7 deleted the fix-fold-reg-mem branch November 28, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants