Stream: git-wasmtime

Topic: wasmtime / issue #4634 Fix sret for AArch64

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2022 at 21:56):

afonso360 commented on issue #4634:

I did some investigation into this, and i think this is being broken by the ensure_struct_return_ptr_is_returned legalization pass.

If I understand this correctly it inserts a second return arg, which is causing some trouble in the regalloc (still don't fully understand why though).

However, by adding that return arg I think it is breaking the AArch64 ABI anyway, so we should probably restrict that to x86. (probably by moving it to the x86 backend as suggested in https://github.com/bytecodealliance/wasmtime/pull/4618#issuecomment-1207227038).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 10 2022 at 12:36):

bjorn3 commented on issue #4634:

I think I found the problem. (i64 sret) -> i64 is legalized to (i64 sret) -> i64 sret, i64, but the return instruction doesn't account for this. This is a pre-existing problem with all backends. It isn't required to work either. In other words I wrote a bad test on which Cranelift failed correctly, but with a confusing error. I will fix the test if at a later point sret legalization is removed in favor of direct integration with the backends, that should fix this issue.

function %f17(i64 sret) -> i64 {
block0(v0: i64):
    return v0
}

view this post on Zulip Wasmtime GitHub notifications bot (Aug 10 2022 at 12:49):

bjorn3 edited a comment on issue #4634:

I think I found the problem. (i64 sret) -> i64 is legalized to (i64 sret) -> i64 sret, i64, but the return instruction doesn't account for this. This is a pre-existing problem with all backends. It isn't required to work either. In other words I wrote a bad test on which Cranelift failed correctly, but with a confusing error. I will fix the test. If at a later point sret legalization is removed in favor of direct integration with the backends, that should fix this issue.

function %f17(i64 sret) -> i64 {
block0(v0: i64):
    return v0
}

view this post on Zulip Wasmtime GitHub notifications bot (Aug 10 2022 at 13:07):

bjorn3 commented on issue #4634:

Apart from i128, all abi-checker tests now pass with this PR.

Last updated: Oct 23 2024 at 20:03 UTC