Closed
Bug 1445272
Opened 6 years ago
Closed 6 years ago
wasm: Implement basic anyref support
Categories
(Core :: JavaScript Engine: JIT, enhancement, P2)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(7 files, 19 obsolete files)
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 |
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
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
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.
Comment 2•6 years ago
|
||
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.
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
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•6 years 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•6 years ago
|
||
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 7•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Attachment #8960662 -
Attachment is obsolete: true
Attachment #8961014 -
Flags: review?(luke)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8961015 -
Flags: review?(luke)
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8961016 -
Flags: review?(luke)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8961017 -
Flags: review?(luke)
Assignee | ||
Comment 13•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Attachment #8961021 -
Attachment is obsolete: true
Assignee | ||
Comment 16•6 years 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•6 years ago
|
||
An extra wip patch for globals, probably will stay on its own due to complexity. Helped me find bug 1449213 \o/
Assignee | ||
Comment 18•6 years 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•6 years ago
|
||
Attachment #8961014 -
Attachment is obsolete: true
Attachment #8963941 -
Flags: review?(luke)
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #8961015 -
Attachment is obsolete: true
Attachment #8961015 -
Flags: review?(luke)
Attachment #8963942 -
Flags: review?(luke)
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #8961488 -
Attachment is obsolete: true
Attachment #8961488 -
Flags: review?(luke)
Attachment #8963943 -
Flags: review?(luke)
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #8961017 -
Attachment is obsolete: true
Attachment #8961017 -
Flags: review?(luke)
Attachment #8963944 -
Flags: review?(luke)
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #8963946 -
Flags: review?(luke)
Assignee | ||
Comment 24•6 years ago
|
||
Attachment #8961489 -
Attachment is obsolete: true
Attachment #8963947 -
Flags: feedback?(lhansen)
Updated•6 years ago
|
Attachment #8963941 -
Flags: review?(luke) → review+
Comment 25•6 years ago
|
||
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 26•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8963944 -
Flags: review?(luke) → review+
Comment 27•6 years ago
|
||
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 28•6 years ago
|
||
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 29•6 years ago
|
||
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 30•6 years ago
|
||
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.
Comment 31•6 years ago
|
||
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.
Comment 32•6 years ago
|
||
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•6 years 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•6 years 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•6 years 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 36•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Carrying forward r+ from Luke (addressed comments, of course).
Attachment #8963943 -
Attachment is obsolete: true
Attachment #8966656 -
Flags: review+
Assignee | ||
Comment 39•6 years ago
|
||
Attachment #8963944 -
Attachment is obsolete: true
Attachment #8966657 -
Flags: review+
Assignee | ||
Comment 40•6 years ago
|
||
Attachment #8963946 -
Attachment is obsolete: true
Attachment #8966658 -
Flags: review+
Assignee | ||
Comment 41•6 years ago
|
||
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 42•6 years ago
|
||
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+
Comment 43•6 years ago
|
||
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•6 years 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•6 years ago
|
||
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•6 years ago
|
||
Carrying forward r+.
Attachment #8966659 -
Attachment is obsolete: true
Attachment #8967086 -
Flags: review+
Assignee | ||
Comment 47•6 years 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 48•6 years ago
|
||
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 49•6 years ago
|
||
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•6 years 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 51•6 years ago
|
||
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•6 years 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.
Assignee | ||
Comment 53•6 years ago
|
||
After a few small fixes, full green try build \o/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef06a081549a86b8972e130a30b4883d17fcd8eb
Comment 54•6 years 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
Comment 55•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/302befe7689a https://hg.mozilla.org/mozilla-central/rev/40baddc5f0b6 https://hg.mozilla.org/mozilla-central/rev/f8b79c586af6 https://hg.mozilla.org/mozilla-central/rev/670d462e97cb https://hg.mozilla.org/mozilla-central/rev/f8a4c128ffd4 https://hg.mozilla.org/mozilla-central/rev/7be1a707d56d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 56•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/302befe7689a https://hg.mozilla.org/mozilla-central/rev/40baddc5f0b6 https://hg.mozilla.org/mozilla-central/rev/f8b79c586af6 https://hg.mozilla.org/mozilla-central/rev/670d462e97cb https://hg.mozilla.org/mozilla-central/rev/f8a4c128ffd4 https://hg.mozilla.org/mozilla-central/rev/7be1a707d56d
You need to log in
before you can comment on or make changes to this bug.
Description
•