You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ze...@apache.org on 2026/10/20 14:02:59 UTC
(arrow-go) branch main updated: fix(parquet/metadata): stop bloom filter asm clobbering the frame pointer (#1379)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-go.git
The following commit(s) were added to refs/heads/main by this push:
new 1eb1d9cc fix(parquet/metadata): stop bloom filter asm clobbering the frame pointer (#1379)
1eb1d9cc is described below
commit 1eb1d9cc632f334a73158c1cbbb9b258f1c76261
Author: Matt Topol <zo...@gmail.com>
AuthorDate: Tue Oct 20 15:02:54 2026 +0100
fix(parquet/metadata): stop bloom filter asm clobbering the frame pointer (#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.
---
parquet/metadata/bloom_filter_block_avx2_amd64.s | 23 ++++++++++----------
parquet/metadata/bloom_filter_block_sse4_amd64.s | 27 ++++++++++++------------
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/parquet/metadata/bloom_filter_block_avx2_amd64.s b/parquet/metadata/bloom_filter_block_avx2_amd64.s
index c176800b..5bd07e74 100644
--- a/parquet/metadata/bloom_filter_block_avx2_amd64.s
+++ b/parquet/metadata/bloom_filter_block_avx2_amd64.s
@@ -1,5 +1,7 @@
//+build !noasm !appengine
-// AUTO-GENERATED BY C2GOASM -- DO NOT EDIT
+// AUTO-GENERATED BY C2GOASM, then hand-edited for go vet: BP is no longer
+// clobbered (constants are addressed via R11) and the bool result is stored
+// with MOVB. Re-apply these edits if regenerating from _lib.
DATA LCDATA1<>+0x000(SB)/8, $0x44974d9147b6137b
DATA LCDATA1<>+0x008(SB)/8, $0xa2b7289d8824ad5b
@@ -8,12 +10,11 @@ DATA LCDATA1<>+0x018(SB)/8, $0x5c6bfb319efc4947
DATA LCDATA1<>+0x020(SB)/8, $0x0000000000000001
GLOBL LCDATA1<>(SB), 8, $40
-TEXT ·_check_block_avx2(SB), $0-32
+TEXT ·_check_block_avx2(SB), $0-25
MOVQ bitset32+0(FP), DI
MOVQ len+8(FP), SI
MOVQ hash+16(FP), DX
- LEAQ LCDATA1<>(SB), BP
WORD $0x8948; BYTE $0xd1 // mov rcx, rdx
LONG $0x20e9c148 // shr rcx, 32
@@ -65,12 +66,12 @@ TEXT ·_check_block_avx2(SB), $0-32
LONG $0x1c87448b // mov eax, dword [rdi + 4*rax + 28]
WORD $0xa30f; BYTE $0xc8 // bt eax, ecx
WORD $0x920f; BYTE $0xd0 // setb al
- MOVQ AX, result+24(FP)
+ MOVB AX, result+24(FP)
RET
LBB0_8:
WORD $0xc031 // xor eax, eax
- MOVQ AX, result+24(FP)
+ MOVB AX, result+24(FP)
RET
TEXT ·_insert_block_avx2(SB), $0-24
@@ -78,7 +79,7 @@ TEXT ·_insert_block_avx2(SB), $0-24
MOVQ bitset32+0(FP), DI
MOVQ len+8(FP), SI
MOVQ hash+16(FP), DX
- LEAQ LCDATA1<>(SB), BP
+ LEAQ LCDATA1<>(SB), R11
LONG $0xc26ef9c5 // vmovd xmm0, edx
LONG $0x20eac148 // shr rdx, 32
@@ -91,10 +92,10 @@ TEXT ·_insert_block_avx2(SB), $0-24
LONG $0x1be8c148 // shr rax, 27
QUAD $0x0003ffffffe0b948; WORD $0x0000 // mov rcx, 17179869152
LONG $0x587de2c4; BYTE $0xc0 // vpbroadcastd ymm0, xmm0
- LONG $0x407de2c4; WORD $0x0045 // vpmulld ymm0, ymm0, yword 0[rbp] /* [rip + .LCPI2_0] */
+ VPMULLD (R11), Y0, Y0 // vpmulld ymm0, ymm0, yword 0[r11] /* [rip + .LCPI2_0] */
WORD $0x2148; BYTE $0xc1 // and rcx, rax
LONG $0xd072fdc5; BYTE $0x1b // vpsrld ymm0, ymm0, 27
- LONG $0x587de2c4; WORD $0x204d // vpbroadcastd ymm1, dword 32[rbp] /* [rip + .LCPI2_1] */
+ VPBROADCASTD 32(R11), Y1 // vpbroadcastd ymm1, dword 32[r11] /* [rip + .LCPI2_1] */
LONG $0x4775e2c4; BYTE $0xc0 // vpsllvd ymm0, ymm1, ymm0
LONG $0x04ebfdc5; BYTE $0x0f // vpor ymm0, ymm0, yword [rdi + rcx]
LONG $0x047ffec5; BYTE $0x0f // vmovdqu yword [rdi + rcx], ymm0
@@ -114,7 +115,7 @@ TEXT ·_insert_bulk_avx2(SB), $0-32
MOVQ block_len+8(FP), SI
MOVQ hashes+16(FP), DX
MOVQ hash_len+24(FP), CX
- LEAQ LCDATA2<>(SB), BP
+ LEAQ LCDATA2<>(SB), R11
WORD $0xc985 // test ecx, ecx
JLE LBB3_4
@@ -126,8 +127,8 @@ TEXT ·_insert_bulk_avx2(SB), $0-32
WORD $0xc989 // mov ecx, ecx
WORD $0xf631 // xor esi, esi
QUAD $0x0003ffffffe0b849; WORD $0x0000 // mov r8, 17179869152
- LONG $0x456ffdc5; BYTE $0x00 // vmovdqa ymm0, yword 0[rbp] /* [rip + .LCPI3_0] */
- LONG $0x587de2c4; WORD $0x204d // vpbroadcastd ymm1, dword 32[rbp] /* [rip + .LCPI3_1] */
+ VMOVDQA (R11), Y0 // vmovdqa ymm0, yword 0[r11] /* [rip + .LCPI3_0] */
+ VPBROADCASTD 32(R11), Y1 // vpbroadcastd ymm1, dword 32[r11] /* [rip + .LCPI3_1] */
LBB3_2:
LONG $0xf20c8b4c // mov r9, qword [rdx + 8*rsi]
diff --git a/parquet/metadata/bloom_filter_block_sse4_amd64.s b/parquet/metadata/bloom_filter_block_sse4_amd64.s
index ac5ff12d..ce34ce14 100644
--- a/parquet/metadata/bloom_filter_block_sse4_amd64.s
+++ b/parquet/metadata/bloom_filter_block_sse4_amd64.s
@@ -1,5 +1,7 @@
//+build !noasm !appengine
-// AUTO-GENERATED BY C2GOASM -- DO NOT EDIT
+// AUTO-GENERATED BY C2GOASM, then hand-edited for go vet: BP is no longer
+// clobbered (constants are addressed via R11) and the bool result is stored
+// with MOVB. Re-apply these edits if regenerating from _lib.
DATA LCDATA1<>+0x000(SB)/8, $0x44974d9147b6137b
DATA LCDATA1<>+0x008(SB)/8, $0xa2b7289d8824ad5b
@@ -9,12 +11,11 @@ DATA LCDATA1<>+0x020(SB)/8, $0x2df1424b705495c7
DATA LCDATA1<>+0x028(SB)/8, $0x5c6bfb319efc4947
GLOBL LCDATA1<>(SB), 8, $48
-TEXT ·_check_block_sse4(SB), $0-32
+TEXT ·_check_block_sse4(SB), $0-25
MOVQ bitset32+0(FP), DI
MOVQ len+8(FP), SI
MOVQ hash+16(FP), DX
- LEAQ LCDATA1<>(SB), BP
WORD $0x8948; BYTE $0xd1 // mov rcx, rdx
LONG $0x20e9c148 // shr rcx, 32
@@ -66,12 +67,12 @@ TEXT ·_check_block_sse4(SB), $0-32
LONG $0x1c87448b // mov eax, dword [rdi + 4*rax + 28]
WORD $0xa30f; BYTE $0xc8 // bt eax, ecx
WORD $0x920f; BYTE $0xd0 // setb al
- MOVQ AX, result+24(FP)
+ MOVB AX, result+24(FP)
RET
LBB0_8:
WORD $0xc031 // xor eax, eax
- MOVQ AX, result+24(FP)
+ MOVB AX, result+24(FP)
RET
TEXT ·_insert_block_sse4(SB), $0-24
@@ -79,7 +80,7 @@ TEXT ·_insert_block_sse4(SB), $0-24
MOVQ bitset32+0(FP), DI
MOVQ len+8(FP), SI
MOVQ hash+16(FP), DX
- LEAQ LCDATA1<>(SB), BP
+ LEAQ LCDATA1<>(SB), R11
LONG $0xc26e0f66 // movd xmm0, edx
LONG $0x20eac148 // shr rdx, 32
@@ -93,18 +94,18 @@ TEXT ·_insert_block_sse4(SB), $0-24
QUAD $0x0003ffffffe0b848; WORD $0x0000 // mov rax, 17179869152
WORD $0x2148; BYTE $0xc8 // and rax, rcx
LONG $0xc0700f66; BYTE $0x00 // pshufd xmm0, xmm0, 0
- LONG $0x4d6f0f66; BYTE $0x00 // movdqa xmm1, oword 0[rbp] /* [rip + .LCPI2_0] */
+ MOVO (R11), X1 // movdqa xmm1, oword 0[r11] /* [rip + .LCPI2_0] */
LONG $0x40380f66; BYTE $0xc8 // pmulld xmm1, xmm0
LONG $0xd1720f66; BYTE $0x1b // psrld xmm1, 27
LONG $0xf1720f66; BYTE $0x17 // pslld xmm1, 23
- LONG $0x556f0f66; BYTE $0x10 // movdqa xmm2, oword 16[rbp] /* [rip + .LCPI2_1] */
+ MOVO 16(R11), X2 // movdqa xmm2, oword 16[r11] /* [rip + .LCPI2_1] */
LONG $0xcafe0f66 // paddd xmm1, xmm2
LONG $0xc95b0ff3 // cvttps2dq xmm1, xmm1
LONG $0x071c100f // movups xmm3, oword [rdi + rax]
WORD $0x560f; BYTE $0xd9 // orps xmm3, xmm1
LONG $0x074c100f; BYTE $0x10 // movups xmm1, oword [rdi + rax + 16]
LONG $0x071c110f // movups oword [rdi + rax], xmm3
- LONG $0x40380f66; WORD $0x2045 // pmulld xmm0, oword 32[rbp] /* [rip + .LCPI2_2] */
+ PMULLD 32(R11), X0 // pmulld xmm0, oword 32[r11] /* [rip + .LCPI2_2] */
LONG $0xd0720f66; BYTE $0x1b // psrld xmm0, 27
LONG $0xf0720f66; BYTE $0x17 // pslld xmm0, 23
LONG $0xc2fe0f66 // paddd xmm0, xmm2
@@ -127,7 +128,7 @@ TEXT ·_insert_bulk_sse4(SB), $0-32
MOVQ block_len+8(FP), SI
MOVQ hashes+16(FP), DX
MOVQ hash_len+24(FP), CX
- LEAQ LCDATA2<>(SB), BP
+ LEAQ LCDATA2<>(SB), R11
WORD $0xc985 // test ecx, ecx
JLE LBB3_4
@@ -139,9 +140,9 @@ TEXT ·_insert_bulk_sse4(SB), $0-32
WORD $0xc989 // mov ecx, ecx
WORD $0xf631 // xor esi, esi
QUAD $0x0003ffffffe0b849; WORD $0x0000 // mov r8, 17179869152
- LONG $0x456f0f66; BYTE $0x00 // movdqa xmm0, oword 0[rbp] /* [rip + .LCPI3_0] */
- LONG $0x4d6f0f66; BYTE $0x10 // movdqa xmm1, oword 16[rbp] /* [rip + .LCPI3_1] */
- LONG $0x556f0f66; BYTE $0x20 // movdqa xmm2, oword 32[rbp] /* [rip + .LCPI3_2] */
+ MOVO (R11), X0 // movdqa xmm0, oword 0[r11] /* [rip + .LCPI3_0] */
+ MOVO 16(R11), X1 // movdqa xmm1, oword 16[r11] /* [rip + .LCPI3_1] */
+ MOVO 32(R11), X2 // movdqa xmm2, oword 32[r11] /* [rip + .LCPI3_2] */
LBB3_2:
LONG $0xf20c8b4c // mov r9, qword [rdx + 8*rsi]