Stream: git-wasmtime

Topic: wasmtime / issue #5463 Delete vconcat/vsplit instructions

Wasmtime GitHub notifications bot (Dec 16 2022 at 23:29):

jameysharp labeled issue #5463:

Feature

The vconcat and vsplit instructions are not currently implemented in any Cranelift backend. At a quick glance through the history, I'm not sure they've ever been implemented, except as a legalization pass that's since been deleted. Let's remove them.

Benefit

We won't have to maintain the instruction definitions or add new rules in the backends to implement these instructions.

Implementation

Remove the instruction definitions from cranelift/codegen/meta/src/shared/instructions.rs and the references to these opcodes from cranelift/interpreter/src/step.rs. I just merged #5462 which implemented vsplit in the interpreter and added a test for it, cranelift/filetests/filetests/runtests/simd-vsplit.clif, which would also need to be deleted. I think that's it!

This would supersede #5457.

Alternatives

We could clarify the semantics of these instructions and implement them in the backends and interpreter, but without a compelling use case it's not clear why we would.

Wasmtime GitHub notifications bot (Dec 16 2022 at 23:29):

jameysharp opened issue #5463:

Feature

The vconcat and vsplit instructions are not currently implemented in any Cranelift backend. At a quick glance through the history, I'm not sure they've ever been implemented, except as a legalization pass that's since been deleted. Let's remove them.

Benefit

We won't have to maintain the instruction definitions or add new rules in the backends to implement these instructions.

Implementation

Remove the instruction definitions from cranelift/codegen/meta/src/shared/instructions.rs and the references to these opcodes from cranelift/interpreter/src/step.rs. I just merged #5462 which implemented vsplit in the interpreter and added a test for it, cranelift/filetests/filetests/runtests/simd-vsplit.clif, which would also need to be deleted. I think that's it!

This would supersede #5457.

Alternatives

We could clarify the semantics of these instructions and implement them in the backends and interpreter, but without a compelling use case it's not clear why we would.

Wasmtime GitHub notifications bot (Dec 16 2022 at 23:29):

jameysharp labeled issue #5463:

Feature

The vconcat and vsplit instructions are not currently implemented in any Cranelift backend. At a quick glance through the history, I'm not sure they've ever been implemented, except as a legalization pass that's since been deleted. Let's remove them.

Benefit

We won't have to maintain the instruction definitions or add new rules in the backends to implement these instructions.

Implementation

Remove the instruction definitions from cranelift/codegen/meta/src/shared/instructions.rs and the references to these opcodes from cranelift/interpreter/src/step.rs. I just merged #5462 which implemented vsplit in the interpreter and added a test for it, cranelift/filetests/filetests/runtests/simd-vsplit.clif, which would also need to be deleted. I think that's it!

This would supersede #5457.

Alternatives

We could clarify the semantics of these instructions and implement them in the backends and interpreter, but without a compelling use case it's not clear why we would.

Wasmtime GitHub notifications bot (Dec 16 2022 at 23:29):

jameysharp labeled issue #5463:

Feature

The vconcat and vsplit instructions are not currently implemented in any Cranelift backend. At a quick glance through the history, I'm not sure they've ever been implemented, except as a legalization pass that's since been deleted. Let's remove them.

Benefit

We won't have to maintain the instruction definitions or add new rules in the backends to implement these instructions.

Implementation

Remove the instruction definitions from cranelift/codegen/meta/src/shared/instructions.rs and the references to these opcodes from cranelift/interpreter/src/step.rs. I just merged #5462 which implemented vsplit in the interpreter and added a test for it, cranelift/filetests/filetests/runtests/simd-vsplit.clif, which would also need to be deleted. I think that's it!

This would supersede #5457.

Alternatives

We could clarify the semantics of these instructions and implement them in the backends and interpreter, but without a compelling use case it's not clear why we would.

Wasmtime GitHub notifications bot (Dec 17 2022 at 10:58):

MozarellaMan commented on issue #5463:

Sounds good to me, I did also have a quick ctrl+f for the instruction and saw it wasn't being used — but didn't know the reason. I'd be happy to pick this up!

Wasmtime GitHub notifications bot (Dec 17 2022 at 14:51):

bjorn3 commented on issue #5463:

The original purpose for the iconcat/isplit and vconcat/vsplit instructions was that it allows legalization to split full-width operations into two half-width operations without having to implement the full-width operations in every backend. So for example a 256bit vector add could be emulated using two 128bit vector adds. After legalization all concat and split instructions would be gone.

Wasmtime GitHub notifications bot (Dec 17 2022 at 17:31):

MozarellaMan commented on issue #5463:

Ah, so would they still be needed then for that use case? Also - what exactly is a "legalization" pass? Is that just a pass that lowers the IR to backend instructions? Sorry if this is a stupid question :)

Wasmtime GitHub notifications bot (Dec 17 2022 at 17:55):

bjorn3 commented on issue #5463:

Legalization is a pass that lowers complex clif ir instructions to simpler ones. For example until recently we had a heap_addr instruction which lowered to a series of clif ir instructions that does bound checking. Most of the work that legalization did has been subsumed by the move to the new backend framework 2 years ago. I believe the plan is to eventually remove it entirely at some point.

Wasmtime GitHub notifications bot (Dec 17 2022 at 19:33):

MozarellaMan commented on issue #5463:

Thanks for the context, that is pretty fascinating. Sorry for more questions, what was different about the new backend framework that made a separate legalisation pass unnecessary?

Wasmtime GitHub notifications bot (Dec 17 2022 at 19:55):

bjorn3 commented on issue #5463:

The old backend framework used clif ir all the way to the end. So high level instructions that are performed using multiple machine instructions needed to be lowered to simpler clif ir instructions that could then be encoded using a single machine instruction each. The new backend framework however lowers from clif ir to a backend specific ir. This lowering can split high level instructions as necessary for the specific architecture, avoiding the need to modify the clif ir during compilation (except for optional optimization passes and the last remaining bits of the legalizer). For example https://github.com/bytecodealliance/wasmtime/blob/93ae9078c5a2588b5241bd7221ace459d2b04d54/cranelift/codegen/src/isa/x64/lower.isle#L92-L103 lowers an iadd.i128 as add + adc.

Wasmtime GitHub notifications bot (Dec 20 2022 at 00:42):

jameysharp closed issue #5463:

Feature

The vconcat and vsplit instructions are not currently implemented in any Cranelift backend. At a quick glance through the history, I'm not sure they've ever been implemented, except as a legalization pass that's since been deleted. Let's remove them.

Benefit

We won't have to maintain the instruction definitions or add new rules in the backends to implement these instructions.

Implementation

Remove the instruction definitions from cranelift/codegen/meta/src/shared/instructions.rs and the references to these opcodes from cranelift/interpreter/src/step.rs. I just merged #5462 which implemented vsplit in the interpreter and added a test for it, cranelift/filetests/filetests/runtests/simd-vsplit.clif, which would also need to be deleted. I think that's it!

This would supersede #5457.

Alternatives

We could clarify the semantics of these instructions and implement them in the backends and interpreter, but without a compelling use case it's not clear why we would.

Last updated: Nov 22 2024 at 16:03 UTC