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

[Wasm GC] ReFinalize when needed in SimplifyGlobals #5682

Merged
merged 3 commits into from
Apr 20, 2023

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 20, 2023

No description provided.

@kripken kripken requested a review from tlively April 20, 2023 20:42
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.

Would it make sense to relax this validation requirement instead? Do we have any other situations where refining the type of a value can cause a validation failure? That seems bad in general. OTOH, do we have other situations where we refine values without refinalizing? If not, we should make this change anyway.

@kripken
Copy link
Member Author

kripken commented Apr 20, 2023

In general refinalizing is the right thing to do, as it lets later optimizations do more, so this is worth landing I think.

But yes, that ref.cast situation is the only case where we actually error. I'd be happier if we removed it (it is odd) but I think your reasoning when you added it was that it would prevent missed optimizations?

@tlively
Copy link
Member

tlively commented Apr 20, 2023

Ok, let's land this and also remove that validation check. If we were consistent about checking that everything had the most precise possible type in the validator, that might be ok, but doing it just for this one instruction is strange.

@kripken kripken merged commit 2cd0f40 into main Apr 20, 2023
@kripken kripken deleted the simplify.globals.refinalize branch April 20, 2023 22:13
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
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