Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s390x: Hard code the link register in all call and return instructions #9361

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Oct 3, 2024

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.

@bjorn3 bjorn3 requested a review from a team as a code owner October 3, 2024 08:18
@bjorn3 bjorn3 requested review from cfallin and removed request for a team October 3, 2024 08:18
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Oct 3, 2024
@cfallin
Copy link
Member

cfallin commented Oct 3, 2024

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?

@uweigand
Copy link
Member

uweigand commented Oct 4, 2024

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.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Oct 4, 2024

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?

@uweigand
Copy link
Member

uweigand commented Oct 4, 2024

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_rr helper 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: #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.

@elliottt
Copy link
Member

elliottt commented Oct 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants