Remove basic reference types#4802
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
This is a draft because the fuzzer is turning up new bugs, but it should be otherwise pretty much done so I thought I'd upload it for review. (I'm out through Monday, so I'll have to fix those bugs next week.) |
This comment was marked as resolved.
This comment was marked as resolved.
| ;; CHECK: (import "env" "get_dataref" (func $get_dataref (param i32 i32 dataref) (result dataref))) | ||
|
|
||
| ;; CHECK: (import "env" "set_dataref" (func $set_dataref (param i32 i32 dataref) (result dataref))) | ||
|
|
There was a problem hiding this comment.
why are these removed but funcref remains in this file?
There was a problem hiding this comment.
Since GC support was incomplete, it seemed simpler to remove what support was there while keeping the tested functionality covering the standardized reference types feature.
| BinaryenPackedTypeInt8: 1 | ||
| BinaryenPackedTypeInt16: 2 | ||
| BinaryenFeatureMVP: 0 | ||
| yenFeatureMVP: 0 |
| Name table; | ||
| Expression* offset; | ||
| Type type = Type::funcref; | ||
| Type type = Type(HeapType::func, Nullable); |
There was a problem hiding this comment.
This works...? I'm surprised C++ allows it. It the function call done once statically, or each time in the constructor?
(this constructor isn't even a constexpr...)
There was a problem hiding this comment.
Yes, default initializers are executed at object construction time. Similarly, default function arguments are evaluated when the function is called.
There was a problem hiding this comment.
Are you not worried about a function call on each construction here? I guess element segments are not that common...
There was a problem hiding this comment.
I am slightly worried about using the Type constructor in more places. It's rather expensive because it has to do a lookup in the global type store. I'll do some quick measurements to check that this isn't a big performance regression.
There was a problem hiding this comment.
Quick measurement didn't show anything concerning.
| bool Type::isNullable() const { | ||
| if (isBasic()) { | ||
| return id >= funcref && id <= eqref; // except i31ref and dataref | ||
| return false; |
Basic reference types like `Type::funcref`, `Type::anyref`, etc. made it easy to accidentally forget to handle reference types with the same basic HeapTypes but the opposite nullability. In principle there is nothing special about the types with shorthands except in the binary and text formats. Removing these shorthands from the internal type representation by removing all basic reference types makes some code more complicated locally, but simplifies code globally and encourages properly handling both nullable and non-nullable reference types.
|
The fuzzer is looking pretty good on this, too 🎉 |
|
It seems this changes break C- and JS- APIs We got with this recent changes |
|
@MaxGraey are you perhaps hard-coding the old values for |
|
Thank for response! Yes they were cached at compile time, not at runtime. Looks like we'll have to refactor that part on our side. |
Basic reference types like
Type::funcref,Type::anyref, etc. made it easy toaccidentally forget to handle reference types with the same basic HeapTypes but
the opposite nullability. In principle there is nothing special about the types
with shorthands except in the binary and text formats. Removing these shorthands
from the internal type representation by removing all basic reference types
makes some code more complicated locally, but simplifies code globally and
encourages properly handling both nullable and non-nullable reference types.