Skip to content

Fix missing Exact type ReFinalization in TypeMerging #7619

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

Merged
merged 3 commits into from
May 23, 2025

Conversation

kripken
Copy link
Member

@kripken kripken commented May 22, 2025

If we merge siblings to their parent, they may end up equal:

(select (result A))
 (B1)
 (B2)
)

=>

(select (result (exact A))) ;; result is now exact
 (A) ;; both are
 (A) ;; now A
)

In such cases we must refinalize, as the LUB is now exact.

@kripken kripken requested a review from tlively May 22, 2025 15:21
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Thanks! I think I have a reproducer for this sitting in my fuzzing directory as well.

// (A) ;; now A
// )
//
ReFinalize().run(getPassRunner(), module);
Copy link
Member

Choose a reason for hiding this comment

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

We could still check whether any merging occurred before refinalizing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done.

if (refinalize) {
ReFinalize().run(getPassRunner(), module);
}
// If we merge siblings, we also need to refinalize because the LUB of merged
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comment about why merging parents can require refinalization as well. (That will make the "also" make more sense here, too.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't follow. The existing comment explained refinalization after merge(Siblings), and the new comment, for merge(Supertypes). Are those not the two cases? I'm not sure what else is unexplained.

@kripken kripken merged commit cdcf62b into WebAssembly:main May 23, 2025
15 checks passed
@kripken kripken deleted the tm.ref branch May 23, 2025 14:21
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.

2 participants