Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Oct 21, 2025

This recently changed in the spec.

  • Add new isCastable() on types.
  • Avoid adding casts on uncastable things in GUFA.
  • Avoid and fix uncastable things in the fuzzer.
  • Handle br_ifs in binary writing using scratch locals when needed.

The br_if change is the only major work. Before, we would use
casts to fix things, as follows:

  • Single values were just cast after the br_if.
  • Tuples were stashed to locals after the br_if, then reloaded+cast.

After, we do this:

  • Single CASTABLE values are just cast after the br_if.
  • Anything else - uncastable, or Tuples - is handled with locals. We
    stash BEFORE the br_if now, then drop the br_if output, then
    reload (this is longer than before due to the drops, but avoids casts).

@kripken kripken requested a review from tlively October 21, 2025 17:42
Comment on lines 1829 to 1834
// Casts can get broken if we replace with something not castable.
if (!with->type.isCastable()) {
if (expr->is<RefCast>() || expr->is<RefTest>() || expr->is<BrOn>()) {
return false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We never have an uncastable type being a subtype of a castable type, so I think this extra check shouldn't be necessary. Am I missing some edge case?

Copy link
Member Author

Choose a reason for hiding this comment

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

We also replace with supertypes in some cases, when we know the parent allows it etc. This does get fired in the fuzzer, if you want I can find a concrete example.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, but we also don't have any uncastable types being supertypes of other types (except unreahcable?). I'm just worried that this is masking a more fundamental bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, in this place we try to replace a child with something arbitrary, so it seems we need this, but actually a few lines below we do check subtyping anyhow. So this is not needed here. If we generalized this to SubtypingDiscoverer (as we do elsewhere) and beyond, maybe we'd need this, but for now I removed it.

}
}

// Fix up casts: Our changes may have put an uncastable type in a cast.
Copy link
Member

Choose a reason for hiding this comment

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

Which changes might have done this?

Copy link
Member Author

Choose a reason for hiding this comment

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

(ditto, this fires for similar reasons, and if you want I can get an example)

Copy link
Member

Choose a reason for hiding this comment

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

That would be helpful, just to make sure there's not a deeper issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

And here, the only possible issue is unreachability, which is safe to ignore. Good catch, I removed this too.

Comment on lines 5672 to 5673
// Avoid continuations in a simple way (this is rare, so being precise is
// not crucial).
Copy link
Member

Choose a reason for hiding this comment

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

Is it only rare because the heap type generator does not yet generate continuation types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, continuations are just one thing among many naturally-occurring function types, and many testcases have GC types...

Copy link
Member

Choose a reason for hiding this comment

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

I would expect that continuation types would become more common as we start to do more with them. Might as well do something we're happy with for the long term here, since we're unlikely to remember to come back and improve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done.

@kripken kripken merged commit 1c4a085 into WebAssembly:main Oct 21, 2025
16 checks passed
@kripken kripken deleted the cont.nocast branch October 21, 2025 23:08
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