Stream: git-wasmtime

Topic: wasmtime / PR #8438 cranelift: Drop unused arguments befo...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:27):

jameysharp opened PR #8438 from jameysharp:drop-unused-args to bytecodealliance:main:

Before this, Cranelift ABI code would emit a stack-load instruction for every stack argument and add all register arguments to the args pseudo-instruction, whether those arguments were used or not.

However, we already know which arguments are used at that point because we need the analysis for load-sinking, so it's easy to filter the unused arguments out.

This avoids generating loads that are immediately dead, which is good for the generated code. It also slightly reduces the size of the register allocation problem, which is a small win in compile time.

This also changes which registers RA2 chooses in some cases because it no longer considers unused defs from the args pseudo-instruction.

There was an existing method named arg_is_needed_in_body which sounded like it should be the right place to implement this. However, that method was only used for Baldrdash integration and has been a stub since that integration was removed in #4571. Also it didn't have access to the value_ir_uses map needed here. But the place where that method was called does have access to that map and was perfect for this.

Thanks to @elliottt for doing the initial investigation of this change with me.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:27):

jameysharp requested abrown for a review on PR #8438.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:27):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8438.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:27):

jameysharp requested alexcrichton for a review on PR #8438.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:27):

jameysharp requested wasmtime-core-reviewers for a review on PR #8438.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 19:32):

cfallin submitted PR review:

Super-clean precision change for a nice result -- thanks very much for thinking through this! I'm excited to see the useless loads go away in a bunch of functions.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 20:37):

jameysharp commented on PR #8438:

So cargo test -p wasmtime engine::serialization::test::cache_accounts_for_opt_level fails with this PR. The panic message is:

thread '<unnamed>' panicked at cranelift/codegen/src/machinst/compile.rs:76:14:
register allocation: SSA(VReg(vreg = 192, class = Int), Inst(1))

This test simply compiles (module (func)) several times with different optimization settings and with Wasmtime's cache enabled.

From experimentation, this problem only seems to occur if Wasmtime's debug-info is enabled. I guess this is the only test that runs in that mode.

I thought emit_value_label_markers_for_block_args, or the special case to call emit_value_label_marks_for_value on the vmctx parameter, seemed like likely candidates for telling RA2 to use an argument that's not otherwise used. But commenting out those calls doesn't fix this.

I'm not sure how else to investigate this; any suggestions?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 20:42):

cfallin commented on PR #8438:

Can you dump the vcode from the failure? The most pertinent bit is figuring out what is still using the argument, I guess (as you suggest)...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 21:06):

jameysharp updated PR #8438.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2024 at 21:49):

jameysharp merged PR #8438.

Last updated: Nov 22 2024 at 17:03 UTC