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

Fix sret for AArch64 #4634

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Fix sret for AArch64 #4634

merged 2 commits into from
Aug 10, 2022

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Aug 7, 2022

AArch64 requires the struct return address argument to be stored in the x8 register. This register is never used for regular arguments.

According to https://github.com/Gankra/abi-checker cg_clif with this PR matches the abi of LLVM for big structs.

Marking as draft as the test fails with:

Register allocation error for vcode
VCode {
  Entry block: 0
Block 0:
    (original IR block: block0)
    (instruction range: 0 .. 6)
  Inst 0: mov %v128, x8
  Inst 1: mov %v129, %v128
  Inst 2: mov %v129, %v128
  Inst 3: mov x8, %v129
  Inst 4: mov x0, %v130
  Inst 5: ret
}

Error: EntryLivein
CLIF for error:
function %f17(i64 sret) -> i64 fast {
block0(v0: i64):
    return v0
}

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. labels Aug 7, 2022
@afonso360
Copy link
Contributor

I did some investigation into this, and i think this is being broken by the ensure_struct_return_ptr_is_returned legalization pass.

If I understand this correctly it inserts a second return arg, which is causing some trouble in the regalloc (still don't fully understand why though).

However, by adding that return arg I think it is breaking the AArch64 ABI anyway, so we should probably restrict that to x86. (probably by moving it to the x86 backend as suggested in #4618 (comment)).

@github-actions github-actions bot added cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:docs labels Aug 9, 2022
@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 10, 2022

I think I found the problem. (i64 sret) -> i64 is legalized to (i64 sret) -> i64 sret, i64, but the return instruction doesn't account for this. This is a pre-existing problem with all backends. It isn't required to work either. In other words I wrote a bad test on which Cranelift failed correctly, but with a confusing error. I will fix the test. If at a later point sret legalization is removed in favor of direct integration with the backends, that should fix this issue.

function %f17(i64 sret) -> i64 {
block0(v0: i64):
    return v0
}

AArch64 requires the struct return address argument to be stored in the x8
register. This register is never used for regular arguments.
@bjorn3 bjorn3 marked this pull request as ready for review August 10, 2022 12:49
@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 10, 2022

Apart from i128, all abi-checker tests now pass with this PR.

@cfallin cfallin merged commit f8c0a88 into bytecodealliance:main Aug 10, 2022
@bjorn3 bjorn3 deleted the aarch64_sret branch August 10, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:docs cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants