Update reference types #3084

dcodeIO · 2020-08-29T02:35:55Z

To align with the current state of the reference types proposal:

Removes nullref
Removes externref and funcref subtyping
A Literal of a nullable reference type can now represent null (previously was type nullref)
Updates the tests and comments out those tests relying on subtyping, to be enabled again in follow-ups

dcodeIO · 2020-08-29T02:44:02Z

This became larger than expected, but this is what was necessary to make stuff work again. Going to add comments tomorrow :)

src/wasm/literal.cpp

… ptr in dumb union

src/ir/ExpressionManipulator.cpp

src/js/binaryen.js-post.js

src/literal.h

dcodeIO · 2020-08-30T22:44:05Z

src/literal.h

  }
+  static Literal makeFunc(Name func) { return Literal(func.c_str()); }
+  static Literal makeExn(ExceptionPackage& package) { return Literal(package); }


First I renamed makeNullref to makeNull for clarity, and then figured that it'd be good to rename all of them because we are not creating types but values.

src/literal.h

src/passes/DeadArgumentElimination.cpp

src/passes/Print.cpp

src/passes/RemoveImports.cpp

src/wasm-binary.h

src/wasm-interpreter.h

src/wasm/literal.cpp

src/wasm/wasm-stack.cpp

src/wasm/wasm-type.cpp

test/exception-handling.wast

test/reference-types.wast

tlively · 2020-08-31T06:20:16Z

@aheejin, I think this overlaps with patches you were working on, but I'm not sure if your work goes further or takes a different approach. Let us know how it makes the most sense to move forward.

aheejin · 2020-08-31T09:54:13Z

Hello. I explicitly asked you that "I started to work on the same patch, but because I didn't want to block you, so if you want another plan or something please let me know". You didn't reply anything to that. I mostly finished (tests have not been updated fully though) my own patch and I was about to upload it.

I'm not sure if this should be the way we cooperate. I think you could've answered my question that you wanted to do this yourself? So that I didn't have to waste my time after all? We don't need to work on the same patch and choose one of them.

I also said I think it'd be good if there is a patch that only updates the reference types up-to-date, and we don't mix it with the ongoing GC implementation, so that the patch is simple and self-contained and well-tested with the fuzzer. This was apparently ignored too, because this patch contains not only reference type changes but all GC stuff. Of course, this was only my opinion and you may disagree, but I really don't understand why you didn't say that then, didn't answer anything that you wanted to implement everything in one patch and also you were gonna do it yourself, and just ignored all my comments.

aheejin · 2020-08-31T10:03:33Z

@aheejin, I think this overlaps with patches you were working on, but I'm not sure if your work goes further or takes a different approach. Let us know how it makes the most sense to move forward.

I don't know. I didn't expect this and I don't think this is a good manner.

He could've said he was gonna implement this himself. I even asked him explicitly. Yeah, I wasted my time, but I can dump my patch, but it is not even easy for me to review this patch without the knowledge of his previous GC patches, because apparently this patch contains not only reference types updates but also GC stuff. I said that the updating patch would be better if it is focused on only reference type updates, and that was my patch's focus, but apparently, that was ignored too. So what should I do? I really don't know. 🤷🏻

Dismissing as requested

tlively

The size of this change (even if much of it is mechanical) makes it difficult to review. I think splitting out the addition of eqref and i31ref everywhere and focusing on just removing nullref would help.

src/binaryen-c.cpp

src/binaryen-c.h

src/literal.h

dcodeIO · 2020-09-07T02:45:32Z

Last commit deletes all the GC prep. If I didn't miss something, this should be reftypes only now.

tlively

Thanks for cutting this down! It felt much easier to review.

src/binaryen-c.cpp

src/ir/manipulation.h

src/tools/fuzzing.h

src/wasm-binary.h

src/wasm/wasm-s-parser.cpp

src/wasm/wasm-type.cpp

test/exception-handling.wast

test/reference-types.wast

test/spec/old_select.wast

tlively

Awesome, I think this is ready to go. Let's hold off on merging it until just before you are ready to create the follow-up PR that re-enables the tests, though. We will have a small period of time where we are not testing subtyping, but we can make that period as short as possible.

tlively · 2020-09-07T21:21:55Z

Also of course make sure you update the commit title and message to reflect the cut-down functionality :)

dcodeIO · 2020-09-07T21:34:57Z

Thanks! The commit title should be up to date, and I can start looking into --enable-anyref next, as suggested above to get subtyping tests up again before diving into GC and the other types. Would expect the diff there to be rather straight-forward, but ofc uncommenting a lot of tests, while adding a few Type::anyref cases here and there with the only subtyping relationship introduced being reftype <: anyref. It's a little hard to start that on current master, though, but I can try to fork it from this PR?

tlively · 2020-09-07T22:50:32Z

Yeah I would start a new branch from this PR, then when everything is good locally with that branch, merge this PR, rebase your local branch onto the new master, and make the next PR.

dcodeIO · 2020-09-08T15:16:02Z

PR on top of this one implementing the anyref feature and enabling the commented out tests again: dcodeIO#4

dcodeIO · 2020-09-09T01:37:52Z

Ran the fuzzer for a bit and looks good. Merging, and continuing with the follow up above :)

Update basic reference types

3b1fc52

dcodeIO commented Aug 29, 2020

View reviewed changes

src/wasm/literal.cpp Outdated Show resolved Hide resolved

fix equality, fix test, make Literal::type immutable, don't use smart…

6800818

… ptr in dumb union