Stream: git-wasmtime

Topic: wasmtime / PR #9190 Refactor `CallInfo` amongst Cranelift...


view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 15:28):

alexcrichton requested fitzgen for a review on PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 15:28):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 15:28):

alexcrichton opened PR #9190 from alexcrichton:refactor-call-info to bytecodealliance:main:

This commit is aimed at making it easier to extend CallInfo amongst the backends with more fields. Most backends already share almost all the same fields except for minor differences so this commit moves backends to all roughly the same track for handling this to enable having one definition of CallInfo that is shared everywhere.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:10):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:10):

fitzgen created PR review comment:

I don't think this is anything we necessarily need to address right now, and some backends are already like this, but by not having the name in the CallInfo we are inflating the size of the instruction a bit. The downside is that now we would need direct vs indirect variants, if we kept everything inside the boxed CallInfo.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:10):

fitzgen created PR review comment:

This seems unrelated?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:10):

fitzgen created PR review comment:

Nevermind, I see that this is factored out from the call implementation

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:16):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 16:16):

bjorn3 created PR review comment:

Maybe this should accept CallInfo pre-boxed? That would save some copying around.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 17:46):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 17:48):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 17:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 17:49):

alexcrichton created PR review comment:

It's true yeah, I was assuming that the x64 style of moving indirect-vs-not into the Inst was ok to apply to all backends. If we end up undoing that though backends could also have something like struct X64CallInfo { info: CallInfo, ... } or similar

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 17:49):

alexcrichton has enabled auto merge for PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 22:45):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 22:45):

alexcrichton has disabled auto merge for PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 22:45):

alexcrichton commented on PR #9190:

Ok, as predicted size assertions got the best of me. I've refactored the other direction for backends to include the name/destination in the CallInfo uniformly across all backends.

That was a large-ish change though so mind taking another look @fitzgen?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 23:09):

fitzgen commented on PR #9190:

I don't have time today, and I'm going backpacking next week, so while I don't mind taking a look when I get back you might want to find another reviewer :-p

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2024 at 23:25):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 14:24):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 14:24):

alexcrichton commented on PR #9190:

@elliottt would you be ok reviewing this?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 18:06):

elliottt submitted PR review:

This all looks good to me!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 18:39):

alexcrichton updated PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 18:39):

alexcrichton has enabled auto merge for PR #9190.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2024 at 19:13):

alexcrichton merged PR #9190.


Last updated: Dec 23 2024 at 12:05 UTC