-
Notifications
You must be signed in to change notification settings - Fork 829
Add new compound Signature, Struct and Array types #3012
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
Conversation
ac5eedd to
185f887
Compare
dcodeIO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my thoughts about the changes made to provide some context.
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
This leads to simpler code and is a prerequisite for #3012, which makes it so that not all `Type`s are backed by vectors that `expand` could return.
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the last round of comments :)
…ement hashing of all the public things
src/wasm-type.h
Outdated
|
|
||
| // Returns true if left is a subtype of right. Subtype includes itself. | ||
| static bool isSubType(Type left, Type right); | ||
| static bool isSubType(const Type& left, const Type& right); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Types are small anyway we are either pushing its uintptr_t id or an equally sized reference, so that's mostly style, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: binaryen master is still in a weird state where we've changed anyref to externref, but the other reference types changes haven't yet been applied; in this case, there is no longer any subtyping relationship, so I assume this method needs to be refactored out as part of removing nullref, adding an immediate type tag to ref.null, etc.
Apologies if this causes issues for folks, I only had spare cycles to do the initial work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder. Plan forward might be:
- Complete this PR so we have a foundation for compound types
- Remove
nullref, addeqref,i31refand the newanyrefalongside initial handling of the new compound types - Think about RTTs
- Update
ref.nullandref.is_nullalongside adding the new GC instructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems good to me. Apologies if it was already top of mind 🤡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @aheejin, who I think was planning to make some changes to reflect the current state of reference types. It would be good to coordinate with her on who is going to make what changes.
On topic here, I think it would be best to keep passing the types directly here. The extra indirection of passing a reference (which means the values must be in memory rather than in registers) is unnecessary. The only reason we use const Type& for operator== and friends is that those are the standard kinds of types used for those operator overloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a half-baked (or, more precisely, quarter-baked) patch for bringing reference types proposal up to date in my local machine, which I started some weeks ago and didn't get around to finish. I'm now trying to pick up the patch.. Do you need that changes for GC? I don't want to block you from proceeding to wait for my patch, but at the same time, I think it is good to add that reference type patch separately from GC and make sure all existing (and to-be-updated) tests the fuzzer pass with reference types proposal, because currently the fuzzer is testing reference type instructions already.
I was away for most of the last week. I was gonna make this change this week, but please let me know if your plan is blocked by this or interfere with this schedule, in which case we can change the plan. I wasn't able to take a look at this patch yet, but I think I'll soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GC proposal restores anyref and nullref, so all we really need to do right now is add an externref type. But some current uses of anyref might need to use externref instead. @aheejin, you'll have a better idea than me whether it's better to rename the current anyref to externref then add a new anyref as part of the GC work or to just add a new externref to what we already have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused.
- Does the GC proposal really restore
nullref? After the recent reference type changes, every reference type can be nullified, or in other words, have a null value. Why would we need to restore a separatenullreftype? I think what you quoted is just not up-to-date, no? - I think we should add
anyref;anyrefandexternrefseem to be two separate types.anyrefis a supertype of all reftypes whileexternrefis not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iiuc there is no nullref anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I've been looking at the wrong GC document. Thanks for the clarification, @dcodeIO!
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with those small changes. Let me know if you agree that it's ready to land :)
Co-authored-by: Thomas Lively <[email protected]>
Co-authored-by: Thomas Lively <[email protected]>
|
This should be consistent in itself even though it doesn't yet address recursive types (probably either mutable compound types one can populate before canonicalization or type pointers) or RTTs (probably a new kind of compound type), but yeah, should be good to merge as a basis for further work :) |
Extends compound types introduced in #3012 with a representation of `Rtt`s as described in the GC proposal, by also introducing the concept of a `HeapType` shared between `TypeInfo` and `Rtt`. Again, this should be a non-functional change since `Rtt`s are not used anywhere yet. Subtyping rules and updating the `xref` aliases is left for future work.
Introduces
TypeInfoas a common descriptor to represent compoundSignature,StructandArraytypes in addition to the existingTupletypes and refactors internal maps to useTypeInfoas their key.