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

Remove StructArgument support from the arm64, riscv64 and s390x backends #9258

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Sep 16, 2024

Unlike x86_64 and some other architectures where large structs are passed at fixed stack offsets, necessitating special Cranelift support, arm64, riscv64 and s390x pass them using regular pointer arguments that can trivially be implemented by frontends.

The current implementation of StructArgument on arm64 and riscv64 uses fixed stack offsets just like on x86_64, which is incorrect as per the ABI documentation. In other words any correct frontend already had to avoid StructArgument on arm64 and riscv64 anyway. For s390x it is correctly implemented but unnecessary and taints even non-s390x specific code.

Unlike x86_64 and some other architectures where large structs are
passed at fixed stack offsets, necessitating special Cranelift support,
arm64, riscv64 and s390x pass them using regular pointer arguments that
can trivially be implemented by frontends.

The current implementation of StructArgument on arm64 and riscv64 uses
fixed stack offsets just like on x86_64, which is incorrect as per the
ABI documentation. In other words any correct frontend already had
to avoid StructArgument on arm64 and riscv64 anyway. For s390x it is
correctly implemented but unnecessary and taints even non-s390x specific
code.
@bjorn3 bjorn3 requested a review from a team as a code owner September 16, 2024 19:07
@bjorn3 bjorn3 requested review from cfallin and removed request for a team September 16, 2024 19:07
@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 16, 2024

All cg_clif tests pass on arm64.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me -- thanks for this simplification!

@cfallin cfallin enabled auto-merge September 16, 2024 19:18
@cfallin cfallin added this pull request to the merge queue Sep 16, 2024
Merged via the queue into bytecodealliance:main with commit c711fcf Sep 16, 2024
69 checks passed
@bjorn3 bjorn3 deleted the abi_cleanup2 branch September 16, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants