Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07): Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07):cfallin requested alexcrichton for a review on PR #8161.
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07):cfallin requested wasmtime-core-reviewers for a review on PR #8161.
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07):cfallin opened PR #8161 from cfallin:typed-funcref-ics-test-update to bytecodealliance:main:
…e table.
This is based on discussion in #8158:
We can use
call_indirectrather thantable.get+call_ref, even on typed funcrefs. TIL; updated the test!As noted in #8160, if we use a nullable typed funcref table instead (and given that we know we'll initialize a particular slot before use on the application side, so we won't actually call a null ref), and if we have a null-ref default value, we should be able to avoid the lazy table-init mechanism entirely.
(Ignore the part where this module doesn't actually have any update logic that would set non-null refs anywhere; it's a compile-test, not a runtest!)
Once #8159 is merged and #8160 is implemented, we should see zero branches in this test.
Of note here is that we do seem to be doing a dynamic signature check again, even though the table has a typed funcref element. Perhaps this path isn't implemented yet either?
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07):cfallin edited PR #8161:
This is based on discussion in #8158:
We can use
call_indirectrather thantable.get+call_ref, even on typed funcrefs. TIL; updated the test!As noted in #8160, if we use a nullable typed funcref table instead (and given that we know we'll initialize a particular slot before use on the application side, so we won't actually call a null ref), and if we have a null-ref default value, we should be able to avoid the lazy table-init mechanism entirely.
(Ignore the part where this module doesn't actually have any update logic that would set non-null refs anywhere; it's a compile-test, not a runtest!)
Once #8159 is merged and #8160 is implemented, we should see zero branches in this test.
Of note here is that we do seem to be doing a dynamic signature check again, even though the table has a typed funcref element. Perhaps this path isn't implemented yet either?
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:07):cfallin edited PR #8161.
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:23):cfallin updated PR #8161.
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:24):cfallin edited PR #8161.
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:24):cfallin edited PR #8161:
This is based on discussion in #8158: as noted in #8160, if we use a nullable typed funcref table instead (and given that we know we'll initialize a particular slot before use on the application side, so we won't actually call a null ref), and if we have a null-ref default value, we should be able to avoid the lazy table-init mechanism entirely.
(Ignore the part where this module doesn't actually have any update logic that would set non-null refs anywhere; it's a compile-test, not a runtest!)
Once #8159 is merged and #8160 is implemented, we should see zero branches in this test.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:24):cfallin commented on PR #8161:
(I switched back from
call_indirecttotable.get+call_refbecause it seems the former doesn't actually omit callee signature checks even when the callee is a typed funcref)
Wasmtime GitHub notifications bot (Mar 17 2024 at 21:27):cfallin edited PR #8161:
This is based on discussion in #8158: as noted in #8160, if we use a nullable typed funcref table instead (and given that we know we'll initialize a particular slot before use on the application side, so we won't actually call a null ref), and if we have a null-ref default value, we should be able to avoid the lazy table-init mechanism entirely.
(Ignore the part where this module doesn't actually have any update logic that would set non-null refs anywhere; it's a compile-test, not a runtest!)
Once #8159 is merged and #8160 is implemented (and table-at-constant-index is fully optimized), we should see zero branches in this test.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Mar 18 2024 at 08:46):jameysharp submitted PR review:
Looks great. The collection of optimizations we're discussing should be easy and should show up very well on this test. I'm looking forward to seeing each one shrink this!
Wasmtime GitHub notifications bot (Mar 18 2024 at 14:55):alexcrichton commented on PR #8161:
it seems the former doesn't actually omit callee signature checks even when the callee is a typed funcref
Ah yes I didn't mean to imply that we already did it, only that it's not too hard to add. I've got a commit after https://github.com/bytecodealliance/wasmtime/pull/8159 which implements the optimization and the codegen for
call_indirectvstable.get+call_refis the same after that.
Wasmtime GitHub notifications bot (Mar 18 2024 at 15:25):alexcrichton merged PR #8161.
Last updated: Nov 22 2024 at 16:03 UTC