Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add component-model-async/fused.wast test #10106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Jan 24, 2025

This is another piece of #9582 which I'm splitting out to make review easier.

This test exercises fused adapter generation for various flavors of intercomponent async->async, async->sync, and sync->async calls.

The remaining changes fill in some TODOs to make the test pass.

@dicej dicej requested a review from alexcrichton January 24, 2025 21:09
@dicej dicej requested a review from a team as a code owner January 24, 2025 21:09
@dicej dicej force-pushed the async-fused-test branch 2 times, most recently from ecd9754 to 7e41099 Compare January 24, 2025 21:33
This is another piece of bytecodealliance#9582 which I'm splitting out to make review easier.

This test exercises fused adapter generation for various flavors of
intercomponent async->async, async->sync, and sync->async calls.

The remaining changes fill in some TODOs to make the test pass.

Signed-off-by: Joel Dice <[email protected]>
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is as far as I'm gonna get today. I haven't gotten to most of trampoline.rs yet which I realize is most of the guts of this PR, but I'll do that on Monday

crates/environ/src/fact/signature.rs Show resolved Hide resolved
crates/environ/src/fact/signature.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/store.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/vm.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/vm.rs Show resolved Hide resolved
Comment on lines +284 to +310
let params = self.types[self.signature]
.unwrap_func()
.params()
.iter()
.map(|&v| {
Some(match v {
WasmValType::I32 => FlatType::I32,
WasmValType::I64 => FlatType::I64,
WasmValType::F32 => FlatType::F32,
WasmValType::F64 => FlatType::F64,
_ => return None,
})
})
.collect::<Option<_>>();

let ty = self.builder.ins().iconst(
ir::types::I32,
i64::from(
params
.and_then(|params| {
self.types
.get_task_return_type(&TypeTaskReturn { params })
.map(|v| v.as_u32())
})
.unwrap_or(u32::MAX),
),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things on this:

  • For let params = ... above, it's unclear to me what this is doing. For example I'm not sure why this would switch to None for unknown types as opposed to return an error or panic or something. Additionally this looks like something that's better done in perhaps a translate.rs or inline.rs step?
  • For let ty = ... this looks like it's doing a hash map lookup of a calculated type. Would it be possible to calculate that when the type is originally created early on in translate.rs and/or inline.rs and then thread that through to here? For example I'd expect that this function here would have ty: Option<SomeIndex> as an argument and then that's what's encoded into a u32 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed about the None default; I don't remember why I did that. I'll switch it to a panic.

And yes, I can move this code to an earlier stage, e.g. translate.rs or inline.rs.

Comment on lines +386 to +392
let call = self.compiler.call_indirect_host(
&mut self.builder,
index,
host_sig,
host_fn,
&callee_args,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above, the call_libcall helper might be able to help clean this up a little further

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my response above: I've already done this in a follow-up PR.

crates/cranelift/src/compiler/component.rs Show resolved Hide resolved
crates/environ/src/fact/trampoline.rs Show resolved Hide resolved
crates/environ/src/fact/trampoline.rs Show resolved Hide resolved
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants