You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2026/09/08 17:33:06 UTC
[PR] fix(parquet/utils): make unpack32 NEON profiler-safe [arrow-go]
zeroshade opened a new pull request, #1300:
URL: https://github.com/apache/arrow-go/pull/1300
### Rationale for this change
Addresses the `_unpack32_neon` part of #983 following the regeneration approach outlined in https://github.com/apache/arrow-go/issues/983#issuecomment-5332134569.
The previous assembly repurposed Go's reserved registers (R18 platform, R27 REGTMP, R28 `g`) and adjusted SP through opaque `WORD` directives, so the assembler's pcsp tables were wrong and `sigprof` could observe a clobbered `g` — truncated/invalid tracebacks, and a crash with `CGO_ENABLED=0`.
### What changes are included in this PR?
- Regenerate `bit_packing_neon_arm64.s` from `_lib/bit_packing_neon.c` with `-ffixed-x18 -ffixed-x27 -ffixed-x28 -fomit-frame-pointer` (per the issue comment, plus omitting the frame). The result is a **leaf function with zero stack usage and zero reserved-register usage**: constants load via `VMOVQ` pseudo-instructions and every branch is a real Go branch instruction, so unwind state is always valid.
- Add `_lib/neon2goasm.py`, which converts the compiled ELF object into the Go assembly and **rejects any instruction touching SP or a reserved register**, plus a `make bit_packing_neon_arm64.s` rule (`C_FLAGS_NEON_GOASM`) so regeneration is reproducible.
- Drop the unused `<string.h>` include so the C compiles freestanding for cross-generation.
- Add `BenchmarkUnpack32` (from the issue) so the packed-decode fast path is exercised under `-cpuprofile`.
- Document the new generation path in `_lib/README.md`.
### Verification
- Semantic equivalence: instruction-by-instruction comparison of the assembled Go function against the source ELF object (branch topology index-normalized, `VMOVQ` constants resolved and compared against `.rodata`) — no differences besides the parameter/return glue.
- `go test ./parquet/internal/utils` (native NEON), `-race`, `-tags noasm`, and `go test -tags assert ./parquet/...` all pass on Apple Silicon.
- The issue's failure mode is fixed: `CGO_ENABLED=0 go test -bench BenchmarkUnpack32 -cpuprofile` no longer crashes, and profiles now show full caller chains:
```text
10ms utils._unpack32_neon
utils.unpack32NEON
utils.(*BitReader).GetBatch
utils.BenchmarkUnpack32
testing.(*B).runN
```
- Throughput improved ~27% as a side effect of the newer codegen (387 → 280 ns/op on `BenchmarkUnpack32`, Apple M4).
### Are there any user-facing changes?
No API changes. CPU profiles of workloads that hit the ARM64 packed decoder are now correct.
Note: `unpack_bool_neon_arm64.s` has a smaller instance of the same pattern (one `WORD`-encoded `stp` with SP writeback); left for a follow-up to keep this reviewable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/utils): make unpack32 NEON profiler-safe [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1300:
URL: https://github.com/apache/arrow-go/pull/1300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org