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

[Seq] Allow presets for more types on firreg #6781

Merged
merged 1 commit into from Mar 5, 2024
Merged

Conversation

nandor
Copy link
Contributor

@nandor nandor commented Mar 4, 2024

No description provided.

@nandor nandor requested a review from seldridge March 4, 2024 14:44
@nandor nandor force-pushed the dev/nandor/seq-preset branch 2 times, most recently from 3ad639c to c5392a5 Compare March 4, 2024 15:53
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Generally looks fine. Can you add tests that check behavior of registers that have 64-but widths (either scalars or aggregates)?

How should this work for aggregates that are larger than 64-bit?

if (hw::type_isa<seq::ClockType>(ty)) {
width = 1;
} else {
int64_t maybeWidth = hw::getBitWidth(ty);
Copy link
Member

Choose a reason for hiding this comment

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

Does this work for 64-bit unsigned types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

The issue I was concerned about is that this was casting an unsigned type returned by hw::getBitWidth to a signed type. However, hw::getBitWidth returns an int64_t so there is no problem. If this was an issue, then this would have to be an absolutely enormous constant...

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Generally looks good.

There seem to be some errors in the checking logic and in the location of error messages. Please fix those before landing.

if (hw::type_isa<seq::ClockType>(ty)) {
width = 1;
} else {
int64_t maybeWidth = hw::getBitWidth(ty);
Copy link
Member

Choose a reason for hiding this comment

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

The issue I was concerned about is that this was casting an unsigned type returned by hw::getBitWidth to a signed type. However, hw::getBitWidth returns an int64_t so there is no problem. If this was an issue, then this would have to be an absolutely enormous constant...

lib/Dialect/Seq/SeqOps.cpp Outdated Show resolved Hide resolved
test/Dialect/Seq/firreg-errors.mlir Outdated Show resolved Hide resolved
@nandor nandor merged commit 23eb8c3 into main Mar 5, 2024
4 checks passed
@nandor nandor deleted the dev/nandor/seq-preset branch March 5, 2024 07:49
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.

None yet

2 participants