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.

Sign up for GitHub

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

Jump to bottom

[FIRRTL][LowerTypes] Keep the order of bundle fields in lowered cat #6376

Merged
merged 1 commit into from
Oct 30, 2024
Merged

[FIRRTL][LowerTypes] Keep the order of bundle fields in lowered cat #6376

merged 1 commit into from
Oct 30, 2024

Conversation

SpriteOvO
Copy link
Member

@SpriteOvO SpriteOvO commented Nov 3, 2023 •
edited
Loading

Fixes #6360.

The first fix #6339 (reverted) caused a regression reported in #6360.

In the first try, I forgot to handle the bit extract operation that follows it, the PR tries to fix it again without regression.

Credit: Thanks to @uenoku for the hint (#6360 (comment))

@SpriteOvO SpriteOvO mentioned this pull request Nov 3, 2023
Closed
@SpriteOvO SpriteOvO marked this pull request as draft November 3, 2023 08:32
@SpriteOvO SpriteOvO force-pushed the keep-aggregate-fields-order-in-cat-again branch from c16a2bb to 3fb8ea7 Compare November 3, 2023 14:47
@SpriteOvO
Copy link
Member Author

SpriteOvO commented Nov 3, 2023 •
edited
Loading

If I understand correctly, it doesn't seem possible to keep the old order specifically for Vector, since a Bundle or Vector can always bitcast to a UInt, in which case we've lost the type used to determine which order should be used. @uenoku What do you think about this? Looks like I just misunderstood something.

This PR also reverses the current order of all bits extract, I'm not very sure if this is an acceptable change? (Will it break something?)

uenoku
uenoku reviewed Nov 3, 2023
View reviewed changes
lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp
@@ -1373,6 +1373,7 @@ bool TypeLoweringVisitor::visitExpr(BitCastOp op) {
if (type_isa<BundleType, FVectorType>(op.getResult().getType())) {
Copy link
Member

Choose a reason for hiding this comment

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

You need to change the direction based on types. We should keep the original order for Vector type.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated, looks like I just misunderstood something. Thanks.

@SpriteOvO SpriteOvO force-pushed the keep-aggregate-fields-order-in-cat-again branch from 3fb8ea7 to 7417fc5 Compare November 3, 2023 15:13
@SpriteOvO SpriteOvO marked this pull request as ready for review November 3, 2023 15:42
uenoku
uenoku reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me but let me check this change internally to make sure 👍

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp Outdated
Comment on lines 1342 to 1346
auto isBundle = [](Type type) {
if (auto refType = type_dyn_cast<RefType>(type))
type = refType.getType();
return type_isa<BundleType>(type);
};
Copy link
Member

Choose a reason for hiding this comment

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

You could use type_isa<BundleType>(getBaseType(type)) but is it possible for RefType to be an operand of BitCast op?

Copy link
Member Author

Choose a reason for hiding this comment

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

This piece of code is copied from the function peelType (link), Since it's already being used as the if condition (link), I guess RefType is possible for op.input?

But op.result doesn't use peelType as the condition, and I should probably replace it with just a type_isa<BundleType>(type).

Copy link
Member

Choose a reason for hiding this comment

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

peelType can be used for a wide range of operations but bitcast op doesn't take ref type as input. So you can drop the ref type handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, dropped it.

@SpriteOvO SpriteOvO force-pushed the keep-aggregate-fields-order-in-cat-again branch from 7417fc5 to 120e792 Compare November 4, 2023 10:06
@SpriteOvO SpriteOvO force-pushed the keep-aggregate-fields-order-in-cat-again branch from 120e792 to 1539593 Compare November 6, 2023 08:24
@SpriteOvO
Copy link
Member Author

Gently ping @uenoku. Any updates on internal checks? Are there any further issues with this PR that need to be fixed?

prithayan
prithayan approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@prithayan prithayan left a comment

Choose a reason for hiding this comment

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

We just realized, this fix would be good to merge. I did some testing, and this looks good. Approving and merging it now.

@prithayan prithayan merged commit c68cbf5 into llvm:main Oct 30, 2024
4 checks passed
@SpriteOvO SpriteOvO deleted the keep-aggregate-fields-order-in-cat-again branch October 31, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uenoku uenoku uenoku left review comments

@prithayan prithayan prithayan approved these changes

@darthscsi darthscsi Awaiting requested review from darthscsi darthscsi is a code owner

@dtzSiFive dtzSiFive Awaiting requested review from dtzSiFive

@seldridge seldridge Awaiting requested review from seldridge seldridge is a code owner

@fabianschuiki fabianschuiki Awaiting requested review from fabianschuiki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FIRRTL] Bitcasts reverse bundle field order in LowerTypes
3 participants
@SpriteOvO @prithayan @uenoku