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?
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.
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?
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?
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.
abrown commented on issue #2398:
It was used at one point but no longer... I would also vote to remove it.
Last updated: Dec 23 2024 at 12:05 UTC