Stream: git-wasmtime

Topic: wasmtime / PR #2030 SPIR-V backend scaffolding

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:23):

Jasper-Bekkers opened PR #2030 from master to master:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:39):

Jasper-Bekkers updated PR #2030 from master to master:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:45):

Jasper-Bekkers updated PR #2030 from master to master:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

Please make these optional and depend on them from the spriv feature.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

#[cfg(feature = "spirv")]
mod spirv;

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

        if self.result_type.is_some() {

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

Does toml support trailing commas? If so, please add one to minimize diffs a bit for future architecture additions.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

Already mentioned below.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

@cfallin Is it still necessary to specify backends in codegen-meta when using the new framework?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

        if self.result_id.is_some() {

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

:wave:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

Something went wrong here. Can you revert this change?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 09:47):

bjorn3 created PR Review Comment:

nit:

        if let Some(result) = self.result_id {

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:04):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:04):

bjorn3 created PR Review Comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:06):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:06):

Jasper-Bekkers created PR Review Comment:

It looks like some more went wrong - not just here, during the rebase on top of main, investigating.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:43):

Jasper-Bekkers updated PR #2030 from master to master:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:51):

Jasper-Bekkers edited PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 11:00):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 11:01):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 11:46):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 11:46):

bnjbvr created PR Review Comment:

It should be only useful to require a new-backend style meta build, that is, build the instructions and expand group. So the module is not strictly required here, the Isa variant below is sufficient, and should be handled in the second loop in codegen/meta/src/lib.rs, probably with an empty match arm (as aarch64) if there are no spirv specific settings. Also the codegen/build.rs script should mimic what's done for x64.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:32):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:32):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:32):

Jasper-Bekkers created PR Review Comment:

Re-rebased on main this time (didn't catch the branch rename the first time around, but it's all fixed now).

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:33):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:33):

Jasper-Bekkers created PR Review Comment:

Seems so, fixed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:33):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:33):

Jasper-Bekkers created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:34):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:34):

Jasper-Bekkers created PR Review Comment:

Changed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:34):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:34):

Jasper-Bekkers created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:38):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 created PR Review Comment:

I think this is can be return an empty IsaRegs.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 created PR Review Comment:

I think Spirv can be handled the same as Arm64.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 created PR Review Comment:

This won't work they way you may think it does. You are only accessing it from a single thread, so it is equivalent to an u32. What about adding a relocation type for ids, so that the code creating a SPIR-V module from all functions can make the ids unique?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:44):

bjorn3 created PR Review Comment:

Reminder to implement.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:46):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:51):

Jasper-Bekkers updated PR #2030 from master to main:

As discussed on Zulip, we're starting to ramp up our efforts on providing the SPIR-V backend, so I thought it might be a good time to provide some insights into the project to the wasmtime team.

I figure it's easier for everybody if we engage early on rather then submit a giant PR in the end, this way we can take it together and one step at a time.

We'll most likely have some changes incoming for internal cranelift datastructures etc to be able to deal with SPIR-V properly but they're not part of this PR yet, we'll try and contribute those as smaller improvements.

Right now the PR is a draft because we'd like to get initial feedback on our approach, because it still contains some debug code and because I need to rebase this on top of master.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers created PR Review Comment:

Yeah; we're having a discussion internally on how to best solve this. We'll need to hand out id's in a few cases; one is to allocate types and the other as virtual SSA registers, so we need some architecture around this.

For type allocation we're thinking of doing a two-pass approach (eg. scan which types are needed and alloc them followed by a pass that just uses them).

For handing out id's during instruction lowering, we'll need some kind of way to have relocateable id's most likely so we'll take a look at that.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers created PR Review Comment:

Replaced with todo!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 12:54):

Jasper-Bekkers created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 09 2020 at 10:00):

Jasper-Bekkers closed without merge PR #2030.

Last updated: Nov 22 2024 at 16:03 UTC