Stream: git-wasmtime

Topic: wasmtime / PR #2376 AArch64 SIMD: replace `LoadSplat` wit...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 00:24):

cfallin requested julian-seward1 for a review on PR #2376.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 00:24):

cfallin opened PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 00:25):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:05):

akirilov-arm submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:05):

akirilov-arm created PR Review Comment:

Shouldn't we keep these helpers in lower.rs? I thought that lower_inst.rs was created to contain just the big functions lower_insn_to_regs() and lower_branch().

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:05):

akirilov-arm created PR Review Comment:

I just had a quick look at #2366, so I might be missing something, but the description says:
This can only be done to an instruction with no uses of its result register(s), because it will cause the instruction not to be codegen'd at its original location.
(I guess the meaning is that there are no uses of its result except the instruction it has been merged into)
It is not clear to me how we ensure that here; at the very least shouldn't there be a test case where the result of a load is used both by a splat and by another operation?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:05):

akirilov-arm submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:53):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2020 at 23:53):

cfallin created PR Review Comment:

Indeed, no other uses than the one by the input merging the op (I'll clarify that, and add a test for the multi-use case).

The condition is ensured by the logic in maybe_get_input_as_source_or_const(): it will return None as the inst field in the returned LowerInput if any of the conditions for code motion are not satisfied. So, things just transparently work in the pattern-matching: we won't see any source op at all if we aren't allowed to merge it.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:05):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:05):

abrown created PR Review Comment:

:+1: for fewer CLIF instructions!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:06):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:06):

abrown created PR Review Comment:

So a separate PR will do the same for x64?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:38):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 17:38):

cfallin created PR Review Comment:

Indeed, that's my plan for today :-)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 18:30):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 18:31):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 18:31):

cfallin created PR Review Comment:

Yes, good point, we've gotten a bit sloppy about this -- moved the added helpers as well as the one above this code.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 18:35):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 09 2020 at 18:37):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 10 2020 at 01:56):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 10 2020 at 19:45):

akirilov-arm submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2020 at 01:18):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2020 at 01:35):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2020 at 23:40):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2020 at 23:59):

cfallin updated PR #2376 from loadsplat to main:

This PR uses the isel changes introduced in #2366 that allows loads to sink and merge with other operations in order to replace the LoadSplat instruction with more general pattern-matching.

LoadSplat was added as an incremental step to improve AArch64 code quality in #2278. At the time, we did not have a way to pattern-match the load + splat opcode sequence that the relevant Wasm opcodes lowered to. However, now with PR #2366, we can merge effectful instructions such as loads into other ops, and so we can do this pattern matching directly.

PR includes commit from #2366 to allow simultaneous review/CI.

cc @julian-seward1 @akirilov-arm @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2020 at 14:12):

julian-seward1 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2020 at 16:03):

cfallin merged PR #2376.

Last updated: Dec 23 2024 at 12:05 UTC