Stream: git-wasmtime

Topic: wasmtime / issue #6237 Bump regalloc2 to 0.7.0

Wasmtime GitHub notifications bot (Apr 18 2023 at 22:08):

cfallin commented on issue #6237:

This will need the is_move method removed from the Function trait impl on VCode, and I think also the corresponding if-statement to skip emitting program moves removed from the VCode emission path (here and [here]https://github.com/bytecodealliance/wasmtime/blob/a486aa37ad187e4b5c5d34da303a39fa5cdd2745/cranelift/codegen/src/machinst/vcode.rs#L929) respectively, and the is_move data structure DCE'd too if it's now dead).

Wasmtime GitHub notifications bot (Apr 18 2023 at 22:09):

cfallin edited a comment on issue #6237:

This will need the is_move method removed from the Function trait impl on VCode, and I think also the corresponding if-statement to skip emitting program moves removed from the VCode emission path (here and here respectively, and the is_move data structure DCE'd too if it's now dead).

Wasmtime GitHub notifications bot (Apr 18 2023 at 22:22):

elliottt commented on issue #6237:

This will need the is_move method removed from the Function trait impl on VCode, and I think also the corresponding if-statement to skip emitting program moves removed from the VCode emission path (here and here respectively, and the is_move data structure DCE'd too if it's now dead).

I think it is unused now, nice! That allows us to remove insertions into the map during collect_operands, and the only use of MachInst::is_move is now for that conditional that guards the two assertions about vregs with moves. I'm inclined to leave that as-is, as I'd like to keep that assertion around, what do you think?

Wasmtime GitHub notifications bot (Apr 18 2023 at 22:42):

cfallin commented on issue #6237:

I think it is unused now, nice! That allows us to remove insertions into the map during collect_operands, and the only use of MachInst::is_move is now for that conditional that guards the two assertions about vregs with moves. I'm inclined to leave that as-is, as I'd like to keep that assertion around, what do you think?

You mean this block of code? That's fine to keep I think, yeah; the is_move at the top there is an invocation of MachInst::is_move(), which is separate from <VCode as regalloc2::Function>::is_move() and is just a match on the instruction. In other words we can remove the is_move hashmap while still keeping this check.

Wasmtime GitHub notifications bot (Apr 18 2023 at 22:50):

elliottt commented on issue #6237:

You mean this block of code? That's fine to keep I think, yeah; the is_move at the top there is an invocation of MachInst::is_move(), which is separate from <VCode as regalloc2::Function>::is_move() and is just a match on the instruction. In other words we can remove the is_move hashmap while still keeping this check.

Yep, I agree with that. I think we could also remove the MachInst::is_move function if we wanted to lose those two assertions. The last two let decls in that block go away as dead code once we remove the table, which is what had me wondering if we still needed MachInst::is_move at all at this point.

Wasmtime GitHub notifications bot (Apr 18 2023 at 23:45):

github-actions[bot] commented on issue #6237:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "winch"

Thus the following users have been cc'd because of the following labels:

• saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:29):

jameysharp commented on issue #6237:

It looks to me like ABI register constraints on incoming args and outgoing return values are now emitting moves, more or less everywhere. Am I interpreting the filetest changes correctly, and is that an expected result? I thought those were all implemented as register constraints on ret and the args pseudo-instruction, not as actual move instructions, so I thought RA2 would still understand how to deal with them.

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:38):

elliottt commented on issue #6237:

It looks to me like ABI register constraints on incoming args and outgoing return values are now emitting moves, more or less everywhere. Am I interpreting the filetest changes correctly, and is that an expected result? I thought those were all implemented as register constraints on ret and the args pseudo-instruction, not as actual move instructions, so I thought RA2 would still understand how to deal with them.

I think it's not so much that they're emitting moves, but that regalloc2 is no longer removing them. This is more or less expected, but I'd like to do a little investigation to see if we can improve things before we merge this.

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:43):

cfallin commented on issue #6237:

I think it's not so much that they're emitting moves, but that regalloc2 is no longer removing them. This is more or less expected, but I'd like to do a little investigation to see if we can improve things before we merge this.

I would say this is very much not expected, and we should not merge with this change. It's pretty significant if we can no longer use the constraints to refer to the args in their original values.

I'd have expected everything to work, because the way that we handle args now is with an args pseudo-inst that defs all the argument vregs with constraints as the very first instruction in the function.

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:44):

cfallin edited a comment on issue #6237:

I think it's not so much that they're emitting moves, but that regalloc2 is no longer removing them. This is more or less expected, but I'd like to do a little investigation to see if we can improve things before we merge this.

I would say this is very much not expected, and we should not merge with this change. It's pretty significant if we can no longer use the constraints to refer to the args in their original values.

I'd have expected everything to work, because the way that we handle args now is with an args pseudo-inst that defs all the argument vregs with constraints as the very first instruction in the function. (In other words, we're no longer emitting any moves here, so RA2 no longer handling moves should be of no consequence: this was the intended order of things, that we remove reliance on handling explicit moves first.)

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:45):

cfallin commented on issue #6237:

(And, as a reward for skimming quickly, I failed to notice that @jameysharp made exactly the same point two comments earlier -- sorry! Yes, we need to fix this before merging.)

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:50):

elliottt commented on issue #6237:

I would say this is very much _not_ expected, and we should not merge with this change. It's pretty significant if we can no longer use the constraints to refer to the args in their original values.

I didn't mean to imply that we should merge with this behavior, just that this output made sense to me given that program moves had been removed in the new version of RA2.

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:52):

cfallin commented on issue #6237:

I would say this is very much _not_ expected, and we should not merge with this change. It's pretty significant if we can no longer use the constraints to refer to the args in their original values.

I didn't mean to imply that we should merge with this behavior, just that this output made sense to me given that program moves had been removed in the new version of RA2.

Right, but the current version of the ABI code doesn't use moves, so it's unexpected that anything should change I think.

Wasmtime GitHub notifications bot (Apr 19 2023 at 00:59):

cfallin commented on issue #6237:

After a little offline discussion with @elliottt it seems what may be happening is that some remaining program-level moves elsewhere in Cranelift's output (i.e., not in ABI code) were still present, and no longer handling these at the regalloc level is what's causing the regressions now. These didn't necessarily go away during SSA-ification because a move is a perfectly reasonable SSA op (namely, it's the identity function). We'll want to remove the last few of these before moving to RA2 0.7.0; it shouldn't be a very large effort.

Wasmtime GitHub notifications bot (Apr 21 2023 at 00:28):

elliottt commented on issue #6237:

/bench_x64

Wasmtime GitHub notifications bot (Apr 21 2023 at 00:36):

jlb6740 commented on issue #6237:

compilation :: cycles :: benchmarks/bz2/benchmark.wasm

Δ = 78635410.00 ± 70514488.64 (confidence = 99%)

main.so is 1.03x to 1.47x faster than commit.so!

[238904854 396078552.64 622424546] commit.so
[218109284 317443142.64 529913904] main.so

instantiation :: cycles :: benchmarks/bz2/benchmark.wasm

Δ = 49379.12 ± 40859.40 (confidence = 99%)

main.so is 1.04x to 1.41x faster than commit.so!

[178906 270460.48 449972] commit.so
[167434 221081.36 300412] main.so

compilation :: cycles :: benchmarks/spidermonkey/benchmark.wasm

No difference in performance.

[7407499152 10121661523.12 17407002596] commit.so
[7004620868 8377593595.52 9898764482] main.so

compilation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm

No difference in performance.

[308082960 444433126.32 1357616580] commit.so
[300892950 372722175.60 532565542] main.so

execution :: cycles :: benchmarks/bz2/benchmark.wasm

No difference in performance.

[113777830 166523657.76 183949430] commit.so
[109460994 144644223.28 188312718] main.so

instantiation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm

No difference in performance.

[201324 251931.76 408750] commit.so
[187242 239093.04 361026] main.so

execution :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm

No difference in performance.

[8336902 10949792.56 29211712] commit.so
[8141614 10515012.32 15191794] main.so

execution :: cycles :: benchmarks/spidermonkey/benchmark.wasm

No difference in performance.

[1023392010 1488396907.12 2048405904] commit.so
[994591294 1539428857.92 2059988336] main.so

instantiation :: cycles :: benchmarks/spidermonkey/benchmark.wasm

No difference in performance.

[494164 615861.28 799082] commit.so
[489712 631441.20 863642] main.so

Wasmtime GitHub notifications bot (Apr 21 2023 at 00:47):

elliottt commented on issue #6237:

The above numbers concerned me, so I ran the bz2 and spidermonkey benchmarks on dedicated hardware. They all showed No difference, so I'm going to attribute the wild difference above to running on CI infrastructure.

Last updated: Nov 22 2024 at 16:03 UTC