subtype-exprs.h additions [NFC] #6323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tlively
reviewed
Feb 20, 2024
src/ir/subtype-exprs.h
Outdated
| @@ -319,7 +338,7 @@ struct SubtypingDiscoverer : public OverriddenVisitor<SubType> { | |||
| auto* seg = self()->getModule()->getElementSegment(curr->segment); | |||
| self()->noteSubtype(seg->type, array.element.type); | |||
| } | |||
| void visitRefAs(RefAs* curr) {} | |||
| void visitRefAs(RefAs* curr) { self()->noteCast(curr->value, curr); } | |||
There was a problem hiding this comment.
This should only be for ref.as_non_null. The internal/external reference conversions are not casts.
|
Yes, I will open a PR right after this with a usage of this. The PR will replace the hacks in StringLowering for nulls with new code that uses this to find which nulls we need to fix. |
|
I forgot to say, and as a result this will be tested in the next PR. Any other thoughts on this one? |
kripken
added a commit
that referenced
this pull request
Feb 27, 2024
) When we do a local.set of a value into a local then we have both a subtyping constraint - for the value to be valid to put in that local - and also a flow of a value, which can then reach more places. Such flow then interacts with casts in Unsubtyping, since it needs to know what can flow where in order to know how casts force us to keep subtyping relations. That regressed in the not-actually-NFC #6323 in which I added the innocuous lines to add subtyping constraints in ref.eq. It seems fine to require that the arms of a RefEq must be of type eqref, but Unsubtyping then assuming those arms flowed into a location of type eqref... which means casts might force us to not optimize some things. To fix this, differentiate the rare case of non-flowing subtyping constraints, which is basically only RefEq. There are perhaps a few more cases (like i31 operations) but they do not matter in practice for Unsubtyping anyhow; I suggest we land this first to undo the regression and then at our leisure investigate the other instructions.
radekdoulik
pushed a commit
to dotnet/binaryen
that referenced
this pull request
Jul 12, 2024
This pulls out the subtype-exprs.h parts of WebAssembly#6108 These are NFC in the current codebase, but are fixes for that unlanded PR, and another unrelated PR that will be opened shortly.
radekdoulik
pushed a commit
to dotnet/binaryen
that referenced
this pull request
Jul 12, 2024
…bAssembly#6344) When we do a local.set of a value into a local then we have both a subtyping constraint - for the value to be valid to put in that local - and also a flow of a value, which can then reach more places. Such flow then interacts with casts in Unsubtyping, since it needs to know what can flow where in order to know how casts force us to keep subtyping relations. That regressed in the not-actually-NFC WebAssembly#6323 in which I added the innocuous lines to add subtyping constraints in ref.eq. It seems fine to require that the arms of a RefEq must be of type eqref, but Unsubtyping then assuming those arms flowed into a location of type eqref... which means casts might force us to not optimize some things. To fix this, differentiate the rare case of non-flowing subtyping constraints, which is basically only RefEq. There are perhaps a few more cases (like i31 operations) but they do not matter in practice for Unsubtyping anyhow; I suggest we land this first to undo the regression and then at our leisure investigate the other instructions.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pulls out the
subtype-exprs.hparts of #6108These are NFC in the current codebase, but are fixes for that unlanded PR. I suggest we
land those because they can help other unrelated work even if we never land that PR.