Stream: git-cranelift

Topic: cranelift / PR #1362 Wasm: Typed select and nullref trans...

GitHub (Jan 23 2020 at 20:17):

eqrion opened PR #1362 from sm/ref-2 to master:

The reftypes proposal has introduced a typed select instruction to ensure efficient validation in the future with complex subtyping relationships. In addition, a proper 'nullref' type has been added. Both of these changes are supported in wasmparser but there are some additional updates needed to cranelift-wasm.

GitHub (Jan 23 2020 at 20:17):

eqrion requested yurydelendik for a review on PR #1362.

GitHub (Jan 23 2020 at 20:51):

fitzgen submitted PR Review.

GitHub (Jan 23 2020 at 20:51):

fitzgen created PR Review Comment:

We should also add a test like

(func (param anyref) (result anyref)
    ref.null
    local.get 0
    i32.const 1
    select (result anyref))

to exercise when the values are distinct subtypes of the typed select's result type. Would be even better if we had enough support for ref.func that we could have turn either a nullref or a funcref into an anyref but I don't think we are there yet, are we?

GitHub (Jan 23 2020 at 20:51):

fitzgen submitted PR Review.

GitHub (Jan 23 2020 at 20:51):

fitzgen created PR Review Comment:

Just to make sure I understand correctly: the reason we don't care about ty is because we already validated the wasm (either in wasmtime or spidermonkey) and know that it is a supertype of both arg1 and arg2, and the builder.ins().select(..) just treats them all reference types as r32 or r64, right?

GitHub (Jan 23 2020 at 21:50):

eqrion created PR Review Comment:

Yes, wasmparser itself contains validation code which will check that arg1 and arg2 are subtypes of the specified type. Spidermonkey uses its own equivalent code. And after validation, there's not a need for a stricter type in Cranelift IR, so it all becomes R32/R64.

I'll add a comment to this effect in the code here.

GitHub (Jan 23 2020 at 21:50):

eqrion submitted PR Review.

GitHub (Jan 23 2020 at 22:01):

eqrion submitted PR Review.

GitHub (Jan 23 2020 at 22:01):

eqrion created PR Review Comment:

Good idea, I'll add that test.

I'm not sure if we can test funcref well. Right now the DummyEnvironment used in wasmtests will convert a wasm ref.func into a clif r32/64.null. So while it would test wasmparser's validation (which has it's own tests), the CL IR would be the same as the other test. (that's if I understand the purpose of wasmtests correctly)

GitHub (Jan 23 2020 at 22:15):

eqrion updated PR #1362 from sm/ref-2 to master:

The reftypes proposal has introduced a typed select instruction to ensure efficient validation in the future with complex subtyping relationships. In addition, a proper 'nullref' type has been added. Both of these changes are supported in wasmparser but there are some additional updates needed to cranelift-wasm.

GitHub (Jan 23 2020 at 22:31):

eqrion merged PR #1362.

Last updated: Nov 22 2024 at 17:03 UTC