zeroshade opened a new pull request, #1379:
URL: https://github.com/apache/arrow-go/pull/1379
### Rationale for this change
`go vet ./parquet/metadata/` reports 12 findings in the c2goasm-generated
bloom filter block kernels (`bloom_filter_block_{avx2,sse4}_amd64.s`):
- **framepointer** (6): each kernel ran `LEAQ LCDATAx<>(SB), BP` in a
frameless function, using BP as the constant-table pointer without saving it.
While the kernel runs, any frame-pointer unwinder (Go's execution tracer,
profilers, perf/eBPF) walks a garbage frame chain.
- **asmdecl** (6): `_check_block_{avx2,sse4}` return a `bool` but declared a
`$0-32` arg frame and stored the result with `MOVQ`. This is harmless in
practice (the extra bytes land in padding) but incorrect.
### What changes are included in this PR?
Hand edits to the two `.s` files. c2goasm is unmaintained, so the header now
says the files are hand-edited:
- `_insert_block_*` / `_insert_bulk_*`: load the constant table into R11 (no
kernel uses it otherwise) instead of BP. The raw-encoded `[rbp]` instructions
become the equivalent Go mnemonics (`VPMULLD`, `VPBROADCASTD`, `VMOVDQA`;
`MOVO`, `PMULLD`). `go tool objdump` shows the same opcodes as before; only the
base register changed.
- `_check_block_*`: these never read the table, so the dead `LEAQ … BP` is
removed. The frame is now `$0-25` and the result is stored with `MOVB`.
### Are these changes tested?
- `go vet ./parquet/metadata/` is now clean (previously 12 findings).
- `go test ./parquet/metadata/ ./parquet/file/` passes on the AVX2 path and
with `GODEBUG=cpu.avx2=off`. Under that setting `BenchmarkFilterInsertBulk`
goes from ~14 to ~22 ns, which confirms the SSE4 kernels are the ones running.
- Performance is neutral (count=10, Intel Core Ultra 7 155H, geomean -0.26%):
```
goos: linux
goarch: amd64
pkg: github.com/apache/arrow-go/v18/parquet/metadata
cpu: Intel(R) Core(TM) Ultra 7 155H
│ /tmp/asm_old.txt │ /tmp/asm_new.txt
│
│ sec/op │ sec/op vs base
│
FilterInsert-22 2.565n ± 1% 2.583n ± 0% ~ (p=0.143
n=10)
FilterCheck-22 3.922n ± 1% 3.877n ± 0% -1.15% (p=0.000
n=10)
FilterCheckBulk-22 78.78n ± 2% 77.39n ± 2% -1.76% (p=0.009
n=10)
FilterInsertBulk-22 14.02n ± 0% 14.22n ± 1% +1.43% (p=0.000
n=10)
SSE4FilterInsert-22 3.152n ± 1% 3.185n ± 1% +1.05% (p=0.000
n=10)
SSE4FilterCheck-22 3.902n ± 0% 3.819n ± 2% -2.15% (p=0.001
n=10)
SSE4FilterCheckBulk-22 78.80n ± 1% 77.64n ± 2% ~ (p=0.063
n=10)
SSE4FilterInsertBulk-22 21.58n ± 1% 21.88n ± 1% +1.37% (p=0.000
n=10)
geomean 11.11n 11.08n -0.26%
│ /tmp/asm_old.txt │ /tmp/asm_new.txt
│
│ B/s │ B/s vs base
│
FilterInsert-22 11.62Gi ± 1% 11.54Gi ± 0% ~ (p=0.165
n=10)
FilterCheck-22 7.599Gi ± 1% 7.687Gi ± 0% +1.16% (p=0.000
n=10)
FilterCheckBulk-22 6.052Gi ± 2% 6.161Gi ± 2% +1.80% (p=0.009
n=10)
FilterInsertBulk-22 34.01Gi ± 0% 33.54Gi ± 1% -1.37% (p=0.000
n=10)
SSE4FilterInsert-22 9.454Gi ± 1% 9.356Gi ± 1% -1.04% (p=0.000
n=10)
SSE4FilterCheck-22 7.637Gi ± 0% 7.805Gi ± 2% +2.20% (p=0.002
n=10)
SSE4FilterCheckBulk-22 6.051Gi ± 1% 6.142Gi ± 2% ~ (p=0.063
n=10)
SSE4FilterInsertBulk-22 22.09Gi ± 1% 21.79Gi ± 1% -1.34% (p=0.000
n=10)
geomean 10.73Gi 10.76Gi +0.27%
```
### Are there any user-facing changes?
No.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
