Wasmtime GitHub notifications bot (Oct 03 2024 at 08:18): Wasmtime GitHub notifications bot (Oct 03 2024 at 08:18):bjorn3 requested cfallin for a review on PR #9361.
Wasmtime GitHub notifications bot (Oct 03 2024 at 08:18):bjorn3 opened PR #9361 from bjorn3:abi_cleanup9 to bytecodealliance:main:
This matches the riscv64 arch which also has the link register defined only by the abi and not by the isa itself. Also remove a couple of dead_code allows.
Wasmtime GitHub notifications bot (Oct 03 2024 at 08:18):bjorn3 requested wasmtime-compiler-reviewers for a review on PR #9361.
Wasmtime GitHub notifications bot (Oct 03 2024 at 23:57):cfallin commented on PR #9361:
cc @uweigand -- this looks reasonable to me overall, and there's no functional change to the emitted code or the regalloc metadata (fields were only ever used with one value) -- but maybe there's a reason it was generic?
Wasmtime GitHub notifications bot (Oct 04 2024 at 20:35):uweigand commented on PR #9361:
Well, as long as the ELF ABI is the only s390x ABI supported by wasmtime, I guess it doesn't make any difference, really.
That said, this seems a step in the wrong direction to me: there are other ABIs used by other OSes on the platforms, and they do use different registers than r14 as the link register, so I'm not sure why it's an improvement to hard-code that value? Logically, it is an ABI property, that's why I placed that choice in ABI code rather than ISA code.
Wasmtime GitHub notifications bot (Oct 04 2024 at 21:05):The ret instruction is also hard coding r15 (stack pointer?) and it has a debug assert that r14 is used as link register. What other OSes use a different link register?
Wasmtime GitHub notifications bot (Oct 04 2024 at 22:06):uweigand commented on PR #9361:
The ret instruction is also hard coding r15 (stack pointer?)
Ah, not really. This instruction uses a condition mask field as "R1", not an actual register field, and the contents are 15 (all mask bits set) to indicate a "branch on any condition", i.e. an unconditional branch. It would have been clearer to add a variant of the
enc_rrhelper instead of passing the condition mask as a "register".and it has a debug assert that r14 is used as link register.
Huh, I'm surprised to see that. Looks like @elliottt added a bunch of asserts here: https://github.com/bytecodealliance/wasmtime/pull/5172 - I'm not sure if there's anything in that diff that makes those asserts necessary?
What other OSes use a different link register?
Well, XPLINK (the current main calling convention on z/OS) uses %r4, as one major example.
In general, I'm pretty sure in case we ever wanted to support a significantly different calling convention like XPLINK that uses different registers for the link register, the stack register, and pretty much any ABI-defined register, we will find places where the current code isn't fully generalized. I tried to keep things general where it seemed easily possible, but didn't attempt any full review ... I just don't see why it's beneficial now to make the code less general.
Wasmtime GitHub notifications bot (Oct 04 2024 at 22:32):elliottt commented on PR #9361:
Huh, I'm surprised to see that. Looks like @elliottt added a bunch of asserts here: #5172 - I'm not sure if there's anything in that diff that makes those asserts necessary?
I think you're right, we should probably remove them. They're added in other cases to check that fixed constraints are respected, but as there's no fixed constraints in the constraint generation code in mod.rs, I don't think that assertion applies.
Wasmtime GitHub notifications bot (Oct 08 2024 at 16:22):uweigand commented on PR #9361:
Huh, I'm surprised to see that. Looks like @elliottt added a bunch of asserts here: #5172 - I'm not sure if there's anything in that diff that makes those asserts necessary?
I think you're right, we should probably remove them. They're added in other cases to check that fixed constraints are respected, but as there's no fixed constraints in the constraint generation code in mod.rs, I don't think that assertion applies.
Thanks. I've now submitted a PR: https://github.com/bytecodealliance/wasmtime/pull/9397
Wasmtime GitHub notifications bot (Oct 17 2024 at 11:35):bjorn3 updated PR #9361.
Wasmtime GitHub notifications bot (Oct 17 2024 at 11:35):bjorn3 edited PR #9361:
The ElfTlsGetOffset pseudo-instruction already hard codes the rest of the abi anyway and if another abi is ever needed, adding another pseudo-instruction or an enum field to the existing one would likely be necessary anyway. Also remove a couple of dead_code allows.
Last updated: Oct 23 2024 at 20:03 UTC