wasm: Implement basic anyref support

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(7 attachments, 19 obsolete attachments)

16.81 KB, patch
luke
: review+
Details | Diff | Splinter Review
11.87 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.25 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
28.80 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
53.20 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
34.94 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
This means implementing the non-controversial parts:

- implementation of text <-> binary changes
- a runtime/build switch (?)
- add support for new types: anyref and nullref
- implement instructions ref.null / ref.is_null
- simple subtyping rules as defined in https://github.com/WebAssembly/reference-types (nullref < anyref)
- pass anyref to wasm functions (entries)
- pass anyref to JS functions (exits)

This wouldn't include:
- table support (including changes to call_indirect, table.get/table.set/table.fill)
- eqref type + ref.eq instruction
- ref.func (resp. ref.host) which construct a reftype from func indexes (resp. host indexes, to be defined by the embedder) + call_ref
(Assignee)

Comment 1

a year ago
Posted patch wip.patch (obsolete) — Splinter Review
WIP patch, which passes basic tests (generates nullref, then checks it's a nullref, and other very interesting stuff like this). Needs more testing, plus needs to comb out all the different parts to make reviews easier.
Just looking at this.  It may be that we really want a lot of the 'Ptr' in the baseline compiler to be 'Ref', because 'Ptr' already has a different meaning there (it means the same as it does in the MacroAssembler, a value that is word-width); I would have preferred 'Word' myself and don't mind changing Ptr -> Word in existing code, but we still have to contend with masm not changing.
I've been discussing with Andreas in https://github.com/WebAssembly/reference-types/pull/3 whether we really need a new null type (relying on subtyping to make this match) vs. a `null $t` opcode where the $t is a value type immediate that indicates which reference type (it'd fail to validate for non reference types).  We agreed to discuss with the broader group in the April CG meeting, so you could implement either way.  I'd be interested if one way was easier to implement than the other, but I was thinking it might be easier to not have the new ValType::Null since ValTypes show up in lots of places.
Since it's hidden by default, link to comment thread on nulltype:
  https://github.com/WebAssembly/reference-types/pull/3#discussion_r174574739
(Assignee)

Comment 5

a year ago
(In reply to Lars T Hansen [:lth] from comment #2)
> Just looking at this.  It may be that we really want a lot of the 'Ptr' in
> the baseline compiler to be 'Ref', because 'Ptr' already has a different
> meaning there (it means the same as it does in the MacroAssembler, a value
> that is word-width); I would have preferred 'Word' myself and don't mind
> changing Ptr -> Word in existing code, but we still have to contend with
> masm not changing.

You're right, will do this, thanks for the suggestion!

(In reply to Luke Wagner [:luke] from comment #3)
> I've been discussing with Andreas in
> https://github.com/WebAssembly/reference-types/pull/3 whether we really need
> a new null type (relying on subtyping to make this match) vs. a `null $t`
> opcode where the $t is a value type immediate that indicates which reference
> type (it'd fail to validate for non reference types).  We agreed to discuss
> with the broader group in the April CG meeting, so you could implement
> either way.  I'd be interested if one way was easier to implement than the
> other, but I was thinking it might be easier to not have the new
> ValType::Null since ValTypes show up in lots of places.

I've started with Andreas' way and then changed to your suggestion later. It only mattered during encoding/decoding (obviously) and during validation. With Andreas' way, I had to use a StackType::NullRef which didn't have any ValType equivalent, so it was tricky and required to specialize each StackType -> ValType conversion to check for NullRef before, which was a bit invasive and error-prone (since catching all of them would have required extensive testing, I think). Your way's only simpler when performing subtyping validation, since we don't have to care about null anymore, viz. null < T for any type T, since now null is typed, we can just remove this part and check vs anyref or just <= T. (viz. MatchesType(LHS, RHS) was defined as LHS is null | RHS is anyref | RHS superclass of LHS; with your suggestion we can remove the first test)
(Assignee)

Comment 6

a year ago
Posted patch wip.patch (obsolete) — Splinter Review
New WIP:

- renames a few high-level concepts to Ref instead of Ptr. Register allocator still uses Ptr, because we're indeed manipulating Ptr at this level, but everything above uses Ref.
- adds support for sync()
Attachment #8960213 - Attachment is obsolete: true
Comment on attachment 8960662 [details] [diff] [review]
wip.patch

Review of attachment 8960662 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +956,5 @@
> +    explicit ScratchPtr(MacroAssembler& m) : ScratchRegisterScope(m) {}
> +    operator RegPtr() const { return RegPtr(Register(*this)); }
> +};
> +# elif JS_CODEGEN_ARM64
> +class ScratchPtr : public vixl::UseScratchRegisterScope

Pretty sure this isn't right - we don't want to use vixl's scratch registers, it uses them all itself.  Does this compile?  vixl::UseScratchRegisterScope has an alloc API that is not generally compatible with our scratch registers.

My gut feeling is that we should equate ScratchPtr and ScratchI32 on all platforms, for the time being, but I'd love to know if there are reasons we can't do that.

There's a broad assumption in the compiler that an I32 register can be widened in place (if necessary) to hold a pointer.  It's in particular true about ScratchI32.  I think it is probably the right thing to have a ScratchPtr because it keeps the semantics clean, but like ScratchDouble aliasing ScratchFloat on several platforms, it'll be OK if ScratchI32 and ScratchPtr alias.

Generally, we should continue to assume that an I32 register can hold a Ptr value.

@@ +8892,5 @@
> +#ifdef JS_PUNBOX64
> +    RegI32 rd = needI32();
> +#else
> +    RegI32 rd = RegI32(r);
> +#endif

Look at narrowI64().  you should be able to introduce a narrowPtr() that takes a RegPtr and returns a RegI32, but that generates no code on all platforms, and then you don't need the ifdef here and below.
(Assignee)

Comment 8

a year ago
Thanks for the early feedback! Indeed I've focused on having x64 work at the moment, will check other platforms tomorrow and investigate your suggestions.
(Assignee)

Comment 9

a year ago
Posted patch 1.wasmgcsupported.patch (obsolete) — Splinter Review
Attachment #8960662 - Attachment is obsolete: true
Attachment #8961014 - Flags: review?(luke)
(Assignee)

Comment 10

a year ago
Posted patch 2.gctypesenabled.patch (obsolete) — Splinter Review
Attachment #8961015 - Flags: review?(luke)
(Assignee)

Comment 11

a year ago
Posted patch 3.anyref-type.patch (obsolete) — Splinter Review
Attachment #8961016 - Flags: review?(luke)
(Assignee)

Comment 12

a year ago
Posted patch 4.text-support.patch (obsolete) — Splinter Review
Attachment #8961017 - Flags: review?(luke)
(Assignee)

Comment 13

a year ago
Posted patch wip.patch (obsolete) — Splinter Review
This last patch needs to be split in baseline-compiler vs other changes, but it's getting interesting: we can now have wasm functions with anyref param/return value, as well as import JS functions with anyref param/return value. (and in tables too, with lazy generation still working)
(Assignee)

Comment 14

a year ago
Posted patch 3.type.system.patch (obsolete) — Splinter Review
Tests have been written and bugs have been found. Then, code has been written and bugs have been fixed.
Attachment #8961016 - Attachment is obsolete: true
Attachment #8961016 - Flags: review?(luke)
Attachment #8961488 - Flags: review?(luke)
(Assignee)

Comment 15

a year ago
Posted patch wip.patch (obsolete) — Splinter Review
Attachment #8961021 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Comment on attachment 8961014 [details] [diff] [review]
1.wasmgcsupported.patch

cancelling review: we figured a shell flag would probably be better than a function to enable it. Then we can have a new tests subdirectory "gc/" under wasm/ tests, which has a directives.txt file that contains --enable-wasm-gc.

Also need to add the pref to Firefox.
Attachment #8961014 - Flags: review?(luke)
(Assignee)

Comment 17

a year ago
Posted patch wip-globals.patch (obsolete) — Splinter Review
An extra wip patch for globals, probably will stay on its own due to complexity. Helped me find bug 1449213 \o/
(Assignee)

Comment 18

a year ago
Comment on attachment 8962761 [details] [diff] [review]
wip-globals.patch

Has its own bug now (it shouldn't be blocking landing here).
Attachment #8962761 - Attachment is obsolete: true
(Assignee)

Comment 19

a year ago
Posted patch 1.shell-flag-pref.patch (obsolete) — Splinter Review
Attachment #8961014 - Attachment is obsolete: true
Attachment #8963941 - Flags: review?(luke)
(Assignee)

Comment 20

a year ago
Posted patch 2.gctypesenabled.patch (obsolete) — Splinter Review
Attachment #8961015 - Attachment is obsolete: true
Attachment #8961015 - Flags: review?(luke)
Attachment #8963942 - Flags: review?(luke)
(Assignee)

Comment 21

a year ago
Posted patch 3.typesystem.patch (obsolete) — Splinter Review
Attachment #8961488 - Attachment is obsolete: true
Attachment #8961488 - Flags: review?(luke)
Attachment #8963943 - Flags: review?(luke)
(Assignee)

Comment 22

a year ago
Posted patch 4.text-to-binary.patch (obsolete) — Splinter Review
Attachment #8961017 - Attachment is obsolete: true
Attachment #8961017 - Flags: review?(luke)
Attachment #8963944 - Flags: review?(luke)
(Assignee)

Comment 23

a year ago
Posted patch 5.stubs.patch (obsolete) — Splinter Review
Attachment #8963946 - Flags: review?(luke)
(Assignee)

Comment 24

a year ago
Posted patch 6.baselinecompiler.patch (obsolete) — Splinter Review
Attachment #8961489 - Attachment is obsolete: true
Attachment #8963947 - Flags: feedback?(lhansen)
Attachment #8963941 - Flags: review?(luke) → review+
Comment on attachment 8963942 [details] [diff] [review]
2.gctypesenabled.patch

Review of attachment 8963942 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmCode.h
@@ +399,5 @@
>  struct MetadataCacheablePod
>  {
>      ModuleKind            kind;
>      MemoryUsage           memoryUsage;
> +    bool                  gcTypesEnabled;

I don't think this field should be necessary.  As is, I see it used in DebugState::debugGetLocalTypes() in the next patch, but I would think that could be replaced by 'true' since we know the binary has already been validated.

::: js/src/wasm/WasmTypes.h
@@ +71,5 @@
>  typedef MutableHandle<WasmTableObject*> MutableHandleWasmTableObject;
>  
>  class WasmGlobalObject;
> +using WasmGlobalObjectVector = GCVector<HeapPtr<WasmGlobalObject*>, 8, SystemAllocPolicy>;
> +using RootedWasmGlobalObject = Rooted<WasmGlobalObject*>;

nit: all the above are typedefs, not 'using', so this looks irregular.  rs=me for batch conversion, though
Attachment #8963942 - Flags: review?(luke) → review+
Comment on attachment 8963943 [details] [diff] [review]
3.typesystem.patch

Review of attachment 8963943 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBinaryIterator.h
@@ +98,5 @@
>  
> +    if (IsRefType(one) && IsRefType(two)) {
> +        if (two == StackType::AnyRef)
> +            return true;
> +        return one == two;

I think you need to set 'result'.  Also, the algorithmic formulation that I think you're looking at here in Andreas's original patch was for 'match()' (which asks the boolean question "is lhs a subtype of rhs?").  Here we're doing something more symmetric: asking if two types have a common subtype ("Meet" might be better).  Incidentally, Andreas had a bug in his original patch that I pointed out:
  https://github.com/WebAssembly/reference-types/pull/3#discussion_r174541667
which he fixed by adding a "meet" and "join":
  https://github.com/WebAssembly/reference-types/pull/3/commits/71435f442e2120bb1483fa5813b2e0859408a889
Practically, I think what we want is: if lhs is AnyRef, result is rhs; if rhs is AnyRef, result is lhs; otherwise, if ==, result is lhs/rhs.

::: js/src/wasm/WasmValidate.h
@@ +698,5 @@
>  MOZ_MUST_USE bool
>  EncodeLocalEntries(Encoder& d, const ValTypeVector& locals);
>  
>  MOZ_MUST_USE bool
> +DecodeLocalEntries(Decoder& d, ModuleKind kind, bool gcTypesEnabled, ValTypeVector* locals);

Given that this bool will stick around for a year or so, could you maybe switch to the stronger-typed HasGcTypes (which also self-comments all the callsites with constant value)?
Attachment #8963943 - Flags: review?(luke) → review+
Attachment #8963944 - Flags: review?(luke) → review+
Comment on attachment 8963946 [details] [diff] [review]
5.stubs.patch

Review of attachment 8963946 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmInstance.cpp
@@ +153,5 @@
> +            JSObject* obj = *(JSObject**)&argv[i];
> +            if (!obj)
> +                args[i].set(NullValue());
> +            else
> +                args[i].set(ObjectValue(*obj));

nit: you can make it a one-liner like the others using ObjectOrNullValue().

@@ +789,5 @@
>      {
> +        // TODO bug 1445277: oh well.
> +        Maybe<gc::AutoSuppressGC> maybeSuppressGc;
> +        if (func.sig().hasAnyrefArgOrRet())
> +            maybeSuppressGc.emplace(cx);

I'm not quite clear on this: would this hunk be removed by bug 1445277?  Given the existence of JIT entries (when an anyref isn't used in the signature, but is used internally), it seems like this AutoSuppressGC wouldn't be worth much.

@@ +888,1 @@
>          args.rval().set(ObjectValue(*retObj));

nit: instead of this, use ObjectOrNullValue()

::: js/src/wasm/WasmTypes.h
@@ +596,5 @@
> +                return true;
> +        }
> +        return false;
> +    }
> +    bool hasAnyrefArgOrRet() const {

nit: could you name this temporarilyUnsupportedAnyRef() or so some such so that, when we add jit entry/exit support, we know to remove this predicate and, transitively, all its uses?

Alternatively, given that jit entries/exits already handle calling out to C++ to do an out-of-line conversion, I wonder how much work it is to just add JIT entry/exit support...
Attachment #8963946 - Flags: review?(luke) → review+
Comment on attachment 8963943 [details] [diff] [review]
3.typesystem.patch

Review of attachment 8963943 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBinaryIterator.cpp
@@ +176,5 @@
>        case Op::F64ConvertSI64:
>        case Op::F64ConvertUI64:
>        case Op::F64ReinterpretI64:
>        case Op::F64PromoteF32:
> +      case Op::RefIsNull:

Drive-by nit: Does this (and other decoding cases, and text->binary) not need ifdefs in addition to the pref?  We probably don't want this to escape off nightly in any form.
Comment on attachment 8963947 [details] [diff] [review]
6.baselinecompiler.patch

Review of attachment 8963947 [details] [diff] [review]:
-----------------------------------------------------------------

Overall very nice.

::: js/src/jit-test/tests/wasm/gc/anyref.js
@@ +248,5 @@
> +
> +let imports = {
> +    i: 0,
> +    myBaguette: null,
> +    anyref: {

How about a different name for this namespace so that we don't get completely confused when reading code below?

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +377,5 @@
>              // only on 64-bit platforms.
>              MOZ_CRASH("AnyReg::any() on 32-bit platform");
>  #endif
> +          case REF:
> +            MOZ_CRASH("no support for gc types in load/store");

Surely not the text we want here?  If this is temporary, at least add some signpost we'll recognize, or a comment.

@@ +931,5 @@
>  #endif
>  
>  #ifdef RABALDR_SCRATCH_I32
> +template<class RegType>
> +class ScratchGPR : public BaseScratchRegister

Nice.

@@ +1291,5 @@
>      void loadLocalI64(const Local& src, RegI64 dest) {
>          masm.load64(Address(sp_, localOffset(src)), dest);
>      }
>  
> +    void loadLocalRef(const Local& src, RegPtr dest) {

This renaming (loadLocalPtr -> loadLocalRef) as well as the one below for storeLocalPtr, is probably not desirable, since 'Ptr' here really means pointer-sized register.  cf that loadStackPtr (which loads a Ptr from the stack) is not renamed, and that loadLocalF64 (for example) loads into a RegF64; here, loadLocalPtr would load into a RegPtr, this seems roughly right to me.

@@ +3214,5 @@
>          MOZ_ASSERT(numval <= stk_.length());
>          for (uint32_t i = stk_.length() - 1; numval > 0; numval--, i--) {
>              Stk& v = stk_[i];
>              switch (v.kind()) {
> +              case Stk::MemRef: MOZ_FALLTHROUGH;

I'd prefer for you to just write out the case in full even though it duplicates the code below.

@@ +8865,5 @@
> +{
> +    RegPtr r = popRef();
> +    RegI32 rd = narrowPtr(r);
> +
> +    masm.cmpPtrSet(Assembler::Equal, r, ImmWord(0), rd);

If you're going to use nullptr above in emitRefNull you should also use it here, ImmWord(intptr_t(nullptr)).  IMO you should instead define global constants to hold any "null ref" values, because I'm unconvinced that we want to scatter info about nullptr and all-zero-bits throughout the module.  How about a NULLREF_INTPTR (obviously we can argue over the name) defined somewhere up above?
Attachment #8963947 - Flags: feedback?(lhansen) → feedback+
Comment on attachment 8963942 [details] [diff] [review]
2.gctypesenabled.patch

Review of attachment 8963942 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmCode.h
@@ +399,5 @@
>  struct MetadataCacheablePod
>  {
>      ModuleKind            kind;
>      MemoryUsage           memoryUsage;
> +    bool                  gcTypesEnabled;

I think I actually need the gcTypesEnabled field in MetadataCacheablePod (or at least in Metadata itself), at least for the time being.  To disable GC when wasm frames are active we need to know, when lazily generating stubs, whether this flag was set or not, and Metadata is the best place to have it.
Just to follow up on that: I think the current solution placing this in MetadataCacheablePod is very suitable, and it would be good to keep it for the time being, but we should perhaps flag it as something we can remove when we remove the gc workaround.
Ah hah, agreed that is a good, if temporary, use for that flag.  +1 to the idea of renaming it to indicate that it's a temporary hack.  Maybe it's name can indicate it's purpose: gcDisableHack?
(Assignee)

Comment 33

a year ago
I know this has already been r+'d, but since we're now having a supplementary #ifdef that leaks into several moz.build (b/o the pref), I'd like to be sure this is the right way to do it.
Attachment #8963941 - Attachment is obsolete: true
Attachment #8966290 - Flags: review?(luke)
(Assignee)

Comment 34

a year ago
Ditto for the second patch.

After this, I'll just carry forward the r+, and will probably not protect many places with ifdefs, because all the gcTypesEnabled checks would have prevented validation in the first place (will audit every path first, but that assumption sounds reasonable).
Attachment #8963942 - Attachment is obsolete: true
Attachment #8966294 - Flags: review?(luke)
(Assignee)

Comment 35

a year ago
Forgot to say: this means more we'd ship dead code until the feature is shipped (bigger code size), but this sounds better than having many ugly #ifdef show up everywhere in the code, and the code size difference should be negligible.
Comment on attachment 8963943 [details] [diff] [review]
3.typesystem.patch

Review of attachment 8963943 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBinaryIterator.h
@@ +98,5 @@
>  
> +    if (IsRefType(one) && IsRefType(two)) {
> +        if (two == StackType::AnyRef)
> +            return true;
> +        return one == two;

What the current spec seems to have here is the join, which is the more conservative unification (it gravitates toward AnyRef, ie least information).  The lenient interpretation in Luke's comment above would not be sound for many uses, I think.
(Assignee)

Comment 37

a year ago
(In reply to Luke Wagner [:luke] from comment #27)
> Comment on attachment 8963946 [details] [diff] [review]
> 5.stubs.patch
> 
> Review of attachment 8963946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmInstance.cpp
> @@ +153,5 @@
> > +            JSObject* obj = *(JSObject**)&argv[i];
> > +            if (!obj)
> > +                args[i].set(NullValue());
> > +            else
> > +                args[i].set(ObjectValue(*obj));
> 
> nit: you can make it a one-liner like the others using ObjectOrNullValue().

So it's subtle: here we can, but for the second case, we also may have a non-null ptr for SIMD objects that have been created. I've renamed the boolean to "expectsObject", so we can do:

if (expectsObject)
  set to ObjectOrNullValue // anyref
else if (ptr)
  set to Object(*ptr) // simd obj
 
> 
> @@ +789,5 @@
> >      {
> > +        // TODO bug 1445277: oh well.
> > +        Maybe<gc::AutoSuppressGC> maybeSuppressGc;
> > +        if (func.sig().hasAnyrefArgOrRet())
> > +            maybeSuppressGc.emplace(cx);
> 
> I'm not quite clear on this [...]

There's a test later that triggered the gc() issue, and putting this was a simple enough hack to hinder the assertion crashing the shell :) Removed it, but then this bug is probably blocked on the gc hack landing first.
(Assignee)

Comment 38

a year ago
Posted patch 3.type-system.patch (obsolete) — Splinter Review
Carrying forward r+ from Luke (addressed comments, of course).
Attachment #8963943 - Attachment is obsolete: true
Attachment #8966656 - Flags: review+
(Assignee)

Comment 39

a year ago
Attachment #8963944 - Attachment is obsolete: true
Attachment #8966657 - Flags: review+
(Assignee)

Comment 40

a year ago
Posted patch 5.stubs.patchSplinter Review
Attachment #8963946 - Attachment is obsolete: true
Attachment #8966658 - Flags: review+
(Assignee)

Comment 41

a year ago
Posted patch 6.rabaldr.patch (obsolete) — Splinter Review
Addressed the renamings. I didn't put the extra ifdef, because I think they're caught by the HasGcTypes condition test, which is false on !ENABLE_WASM_GC platforms.
Attachment #8963947 - Attachment is obsolete: true
Attachment #8966659 - Flags: review?(lhansen)
Comment on attachment 8966659 [details] [diff] [review]
6.rabaldr.patch

Review of attachment 8966659 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still undecided about the Ptr vs Ref changes in general but it's senseless to block this patch until I can make up my mind.  Let's just land this and figure it out later.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +2850,5 @@
>          stk_.popBack();
>          return specific;
>      }
>  
> +    // Call only from other popPtr() variants.

"popRef"

@@ +9736,5 @@
>              CHECK_NEXT(emitGrowMemory());
>            case uint16_t(Op::CurrentMemory):
>              CHECK_NEXT(emitCurrentMemory());
>  
> +          case uint16_t(Op::RefNull):

I still would very much like for these two new opcodes to be under the ifdef.  Defence in depth and all that- we don't want some later change to how the flag is handled, or a bug in how it is handled, open up a security hole.  IMO the verifier should also have the ifdefs for the same reason.
Attachment #8966659 - Flags: review?(lhansen) → review+
The fix to anyref.js is required because wasm.js is not loaded automatically.

The fix to binary.js is required because the opcodes are valid; I tried to make this be aware of whether wasmGcEnabled() is true but it was a mess.  This is good enough.

The #ifdefs in the other two files are optional but I think we need them (as I said in an earlier comment).
Flags: needinfo?(bbouvier)
(Assignee)

Comment 44

a year ago
Thanks for the adjustments; I think I forgot to add a directives.txt file to the test patch.
Flags: needinfo?(bbouvier)
(Assignee)

Comment 45

a year ago
Posted patch 3.type-system.patch (obsolete) — Splinter Review
Small changes in the type system patch, where Unify must actually be aware of whether gc types are enabled or not. For block types we should check also if AnyRef is allowed (I added test cases in the final patch of this bug).
Attachment #8966656 - Attachment is obsolete: true
Attachment #8967084 - Flags: review?(luke)
(Assignee)

Comment 46

a year ago
Carrying forward r+.
Attachment #8966659 - Attachment is obsolete: true
Attachment #8967086 - Flags: review+
(Assignee)

Comment 47

a year ago
(In reply to Lars T Hansen [:lth] from comment #43)
> The fix to binary.js is required because the opcodes are valid; I tried to
> make this be aware of whether wasmGcEnabled() is true but it was a mess. 
> This is good enough.

Didn't include this change, because if --wasm-gc isn't passed to the test, then the opcodes are indeed invalid.
Comment on attachment 8966290 [details] [diff] [review]
1.shell-flag-pref.patch

Review of attachment 8966290 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8966290 - Flags: review?(luke) → review+
Comment on attachment 8966294 [details] [diff] [review]
2.gctypesenabled.patch

Review of attachment 8966294 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8966294 - Flags: review?(luke) → review+
(Assignee)

Comment 50

a year ago
Sorry for the back and forth here, I forgot to address Lars' comment about join.

So this new patch is the same as the one Luke reviewed, + changes to Join/Unify:

- renamed "one"/"two" from Unify parameters to "observed"/"expected", because these can now be asymmetric with ref types.
- introduced a helper to test that a reftype is a subtype of another one. At the moment it's trivial, of course.
- Unify with reftypes just want to test whether observed is a subtype of expected (we use Unify when we try to pop a type we expect, eg checking a returned value has the expected return type).
- Join implements the greatest lower bound, and is used for select: select(T, T') where T <= T' will return T'.
Attachment #8967084 - Attachment is obsolete: true
Attachment #8967084 - Flags: review?(luke)
Attachment #8967416 - Flags: review?(luke)
Comment on attachment 8967416 [details] [diff] [review]
3.type-system.patch

Review of attachment 8967416 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBinaryIterator.h
@@ +86,5 @@
> +    return one == two || two == StackType::AnyRef;
> +}
> +
> +static inline bool
> +Unify(HasGcTypes gcTypesEnabled, StackType observed, StackType expected, StackType* result)

If we have a Join(), I think this should be named Meet() (which was perhaps a better name to begin with)

@@ +117,5 @@
> +static inline bool
> +Join(HasGcTypes gcTypesEnabled, StackType one, StackType two, StackType* result)
> +{
> +    if (gcTypesEnabled == HasGcTypes::False || !IsRefType(one) || !IsRefType(two))
> +        return Unify(gcTypesEnabled, one, two, result);

In the spec version of these algorithms:
  https://webassembly.github.io/reference-types/core/appendix/algorithm.html
the join() doesn't call meet(), so could you do likewise and spell it all out here?
Attachment #8967416 - Flags: review?(luke) → review+
(Assignee)

Comment 52

a year ago
Thanks for the review!

(In reply to Luke Wagner [:luke] from comment #51)
> Comment on attachment 8967416 [details] [diff] [review]
> 3.type-system.patch
> 
> Review of attachment 8967416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmBinaryIterator.h
> @@ +86,5 @@
> > +    return one == two || two == StackType::AnyRef;
> > +}
> > +
> > +static inline bool
> > +Unify(HasGcTypes gcTypesEnabled, StackType observed, StackType expected, StackType* result)
> 
> If we have a Join(), I think this should be named Meet() (which was perhaps
> a better name to begin with)

I'm not convinced, actually: I think Unify is the equivalent of Matches() (which really is a generalized IsSubtypeOf), not Meet(). For instance the Meet algorithm as spelled in the spec, when called with (Tsub, T) where Tsub <= T, will return true and set result to Tsub. Not sure this is what we want. But actually, not even sure this outparam is useful... Will defer investigation/renaming to another bug.

> @@ +117,5 @@
> > +static inline bool
> > +Join(HasGcTypes gcTypesEnabled, StackType one, StackType two, StackType* result)
> > +{
> > +    if (gcTypesEnabled == HasGcTypes::False || !IsRefType(one) || !IsRefType(two))
> > +        return Unify(gcTypesEnabled, one, two, result);
> 
> In the spec version of these algorithms:
>   https://webassembly.github.io/reference-types/core/appendix/algorithm.html
> the join() doesn't call meet(), so could you do likewise and spell it all
> out here?

Sure.

Comment 54

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/302befe7689a
Add a pref to enable wasm gc in the browser/shell; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/40baddc5f0b6
Add gcTypesEnabled to wasm::ModuleEnvironment and Metadata; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b79c586af6
Add Anyref to the wasm type system; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/670d462e97cb
Implement text-to-binary support for anyref; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a4c128ffd4
Add (entry/exit) stubs support for anyref; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be1a707d56d
Implement basic anyref support in the baseline compiler; r=lth
(Assignee)

Updated

a year ago
Depends on: 1454923
You need to log in before you can comment on or make changes to this bug.