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

[NFC + bugfix] Remove BreakTargetLocation from GUFA #6956

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Sep 18, 2024

Before, a br would send its value to a BreakTargetLocation. That would then be
linked to the target:

{ br's value } => BreakTargetLocation(target name) => { location of target }

This PR skips the middle:

{ br's value } => { location of target }

It just connects breaks directly to the targets. We can do that if we keep a
map of the targets as we go.

This is 2% faster as well as simplifies the code, as an NFC refactoring. But it
also fixes a bug: we have handling on ExpressionLocation that filters values as they
come in (they must accord with the expression's type). We were not doing
that on BreakTargetLocation, leading to an assert. Removing
BreakTargetLocation entirely is easier and better than adding filtering logic
for it.

Fixes #6955

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.

Code looks good. Can we add a test for the bugfix?

@kripken
Copy link
Member Author

kripken commented Sep 18, 2024

Existing tests on filtering of expression content should cover this. The connection changes (that breaks go to the right place) are also heavily tested. With all that said, I would add a somewhat-redundant testcase here, but reducing the original testcase turns out to be quite difficult.

@kripken kripken merged commit 5e4a4ba into WebAssembly:main Sep 18, 2024
13 checks passed
@kripken kripken deleted the pc.noBT branch September 18, 2024 22:55
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.

Assert (!contents.isMany()), function updateContent on kotlin/wasm generated wasm file
2 participants