Stream: git-wasmtime

Topic: wasmtime / issue #2398 Should `const_addr` be removed?

Wasmtime GitHub notifications bot (Oct 01 2021 at 21:27):

akirilov-arm labeled issue #2398:

While implementing packed shifts for the old backend, I needed a way to be able to determine the memory address o an offset into a constant emitted by Cranelift. To do this, I added const_addr, an instruction that returns the base address of where the constant is emitted in memory. This pattern followed similar instructions in Cranelift: table_addr, func_addr, stack_addr, heap_addr. In the new backend, I can lower packed shifts without this instruction: since I have more direct control of the compiler machinery, I can figure out the location of the constant in the buffer and emit the appropriate machine-level instructions. const_addr exists because in the old backend this had to be done using CLIF instructions.

I am debating whether to remove this instruction:

• in favor of removing it, fewer instructions would be nice and the primary purpose of this instruction can be fulfilled at a different level
• in favor of keeping it, Cranelift frontend users might find it useful for mask tables (like I did) and the instruction is symmetrical to other *_addr instructions Cranelift provides.

Any strong opinions one way or the other?

Wasmtime GitHub notifications bot (May 04 2022 at 23:05):

cfallin commented on issue #2398:

Coming back to this issue during some gardening: with fresh eyes, it seems to me at least that const_addr is reasonable and there are legitimate use-cases, especially now that we have a better-developed constant pool infrastructure. I think I'll go ahead and close this as there's no pressing need or argument to remove the instruction.

Wasmtime GitHub notifications bot (May 04 2022 at 23:05):

cfallin closed issue #2398:

While implementing packed shifts for the old backend, I needed a way to be able to determine the memory address o an offset into a constant emitted by Cranelift. To do this, I added const_addr, an instruction that returns the base address of where the constant is emitted in memory. This pattern followed similar instructions in Cranelift: table_addr, func_addr, stack_addr, heap_addr. In the new backend, I can lower packed shifts without this instruction: since I have more direct control of the compiler machinery, I can figure out the location of the constant in the buffer and emit the appropriate machine-level instructions. const_addr exists because in the old backend this had to be done using CLIF instructions.

I am debating whether to remove this instruction:

• in favor of removing it, fewer instructions would be nice and the primary purpose of this instruction can be fulfilled at a different level
• in favor of keeping it, Cranelift frontend users might find it useful for mask tables (like I did) and the instruction is symmetrical to other *_addr instructions Cranelift provides.

Any strong opinions one way or the other?

Wasmtime GitHub notifications bot (Sep 01 2022 at 01:16):

jameysharp commented on issue #2398:

I was looking at old open PRs, and #2399 prompted me to look at this. Given that no backend actually implements const_addr today, is it still worth having?

Wasmtime GitHub notifications bot (Sep 01 2022 at 20:53):

cfallin commented on issue #2398:

Hmm, yeah, I think my reasoning about was that it was a consistent and sensible instruction to have; but actually, if it's not implemented and hasn't been missed yet, maybe we can just remove it for now. The constant pool is still there (and used by our lowerings for other instructions internally); if we want to expose the ability to have an arbitrary blob of data in the pool we can always actually implement this later. So I'd be happy to r+ a PR to remove it.

Wasmtime GitHub notifications bot (Sep 01 2022 at 20:59):

abrown commented on issue #2398:

It was used at one point but no longer... I would also vote to remove it.

Last updated: Nov 22 2024 at 16:03 UTC