purego: pass non-HFA arm64 structs in integer registers - #517
Conversation
|
Fix the test fail |
placeRegistersArm64 routed float/64-bit members straight to FP or
integer registers by kind, so mixed structs such as {int64; float64}
went out on x0/v0 while AAPCS64 (and our own getCallbackStruct)
expect them packed into x0/x1. Copy the in-memory image eightbyte by
eightbyte for non-HFA/HVA aggregates of 16 bytes or less.
fcacd50 to
7f5aed1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new packing path can still split a single struct across registers and stack on integer-register overflow, conflicting with the file’s own all-or-nothing overflow handling in getCallbackStruct.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adjusts ARM64 (non-Darwin) struct argument register placement to match AAPCS64 for small non-HFA/HVA aggregates, fixing mixed-member structs that were previously split across GPR/FPR.
Changes:
- Adds a non-HFA/HVA
<=16-byte fast path that copies the struct’s in-memory image in 8-byte chunks into integer registers. - Avoids routing fields to FP vs integer registers purely by kind for these small non-HFA/HVA aggregates.
File summaries
| File | Description |
|---|---|
| struct_arm64.go | Packs small non-HFA/HVA aggregates into 1–2 integer-register chunks based on in-memory layout (ARM64 ABI alignment). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…stack
addStruct packed small non-HFA/HVA structs into integer registers
eightbyte by eightbyte, so when fewer registers than eightbytes
remained, the struct was split across the last register and the stack.
AAPCS64 and getCallbackStruct instead pass such a struct entirely on
the stack, so exhaust the integer registers first to force whole-struct
stack placement (Darwin keeps its splitting convention).
Also add arm64 round-trip tests for struct{int64; double}, including
the register-overflow case, to cover the ebitengine#522 regression.
|
Addressed both review comments:
Verified under qemu-aarch64 against real compiled C: the new tests fail on the pre-fix code (float dropped to v0 / struct split across x7+stack) and pass now. The full test suite passes on linux/amd64 and on linux/arm64 (CGO enabled and disabled). Tests should be green. |
| if ret := fn(expected); ret != expected { | ||
| t.Fatalf("IdentityInt64AndDouble returned %+v wanted %+v", ret, expected) | ||
| } | ||
| if runtime.GOOS != "darwin" { |
There was a problem hiding this comment.
What happens with the below function on Darwin?
What issue is this addressing?
Closes #522
What type of issue is this addressing?
bug
What this PR does | solves
placeRegistersArm64 routed float/64-bit members straight to FP or integer registers by kind, so mixed structs such as {int64; float64} went out on x0/v0 while AAPCS64 (and our own getCallbackStruct) expect them packed into x0/x1. Copy the in-memory image eightbyte by eightbyte for non-HFA/HVA aggregates of 16 bytes or less.