diff --git a/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s b/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s index 3d56efc51..d6cd2070a 100644 --- a/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s +++ b/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s @@ -10,13 +10,7 @@ TEXT ·_cast_type_numeric_neon(SB), $0-40 MOVD len+32(FP), R4 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100181f // cmp w0, #6 - WORD $0x910003fd // mov x29, sp BGT LBB0_17 WORD $0x71000c1f // cmp w0, #3 @@ -4450,9 +4444,6 @@ LBB0_892: WORD $0xb800452b // str w11, [x9], #4 BNE LBB0_892 LBB0_893: - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET LBB0_894: WORD $0x927b6909 // and x9, x8, #0xffffffe0 diff --git a/arrow/math/int64_neon_arm64.s b/arrow/math/int64_neon_arm64.s index c0b639bd2..6eafdd187 100644 --- a/arrow/math/int64_neon_arm64.s +++ b/arrow/math/int64_neon_arm64.s @@ -10,13 +10,7 @@ TEXT ·_sum_int64_neon(SB), $0-24 MOVD buf+0(FP), R0 MOVD len+8(FP), R1 MOVD res+16(FP), R2 - - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! - WORD $0x910003fd // mov x29, sp + CBZ R1, LBB0_3 WORD $0xf1000c3f // cmp x1, #3 BHI LBB0_4 @@ -26,9 +20,6 @@ TEXT ·_sum_int64_neon(SB), $0-24 LBB0_3: WORD $0xaa1f03e9 // mov x9, xzr WORD $0xf9000049 // str x9, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET LBB0_4: WORD $0x927ef428 // and x8, x1, #0xfffffffffffffffc @@ -59,8 +50,5 @@ LBB0_8: BNE LBB0_8 LBB0_9: WORD $0xf9000049 // str x9, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET diff --git a/arrow/math/uint64_neon_arm64.s b/arrow/math/uint64_neon_arm64.s index 0f8d66a5b..4b869972e 100644 --- a/arrow/math/uint64_neon_arm64.s +++ b/arrow/math/uint64_neon_arm64.s @@ -10,13 +10,7 @@ TEXT ·_sum_uint64_neon(SB), $0-24 MOVD buf+0(FP), R0 MOVD len+8(FP), R1 MOVD res+16(FP), R2 - - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! - WORD $0x910003fd // mov x29, sp + CBZ R1, LBB0_3 WORD $0xf1000c3f // cmp x1, #3 BHI LBB0_4 @@ -26,9 +20,6 @@ TEXT ·_sum_uint64_neon(SB), $0-24 LBB0_3: WORD $0xaa1f03e9 // mov x9, xzr WORD $0xf9000049 // str x9, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET LBB0_4: WORD $0x927ef428 // and x8, x1, #0xfffffffffffffffc @@ -59,8 +50,5 @@ LBB0_8: BNE LBB0_8 LBB0_9: WORD $0xf9000049 // str x9, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET diff --git a/arrow/memory/memory_neon_arm64.s b/arrow/memory/memory_neon_arm64.s index 18b0af5c0..19dd7db32 100644 --- a/arrow/memory/memory_neon_arm64.s +++ b/arrow/memory/memory_neon_arm64.s @@ -11,14 +11,8 @@ TEXT ·_memset_neon(SB), $0-24 MOVD len+8(FP), R1 MOVD c+16(FP), R2 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x8b010008 // add x8, x0, x1 WORD $0xeb00011f // cmp x8, x0 - WORD $0x910003fd // mov x29, sp BLS LBB0_7 WORD $0xf100803f // cmp x1, #32 @@ -43,7 +37,4 @@ LBB0_6: WORD $0xeb09011f // cmp x8, x9 BNE LBB0_6 LBB0_7: - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET diff --git a/arrow/memory/memory_neon_arm64_test.go b/arrow/memory/memory_neon_arm64_test.go new file mode 100644 index 000000000..c4db665e4 --- /dev/null +++ b/arrow/memory/memory_neon_arm64_test.go @@ -0,0 +1,94 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file to you under the +// Apache License, Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build arm64 && !noasm + +package memory_test + +import ( + "os" + "os/exec" + "runtime/pprof" + "strings" + "testing" + "time" + + "github.com/apache/arrow-go/v18/arrow/memory" +) + +func TestNEONAssemblyTracebacks(t *testing.T) { + // This is a regression test for GH-983. _memset_neon used to modify the + // stack pointer in a way that the assembler didn't see, leading to + // broken tracebacks in CPU profiler samples when the function is + // running. + + buf := make([]byte, 1<<20) + var traces []byte + duration := time.Second + d := t.TempDir() + for range 5 { + f, err := os.CreateTemp(d, "cpu.prof") + if err != nil { + t.Fatal(err) + } + if err := pprof.StartCPUProfile(f); err != nil { + f.Close() + t.Skipf("CPU profiling is already enabled: %v", err) + } + + deadline := time.Now().Add(duration) + for time.Now().Before(deadline) { + memory.Set(buf, 0x1f) + } + pprof.StopCPUProfile() + if err := f.Close(); err != nil { + t.Fatal(err) + } + + traces, err = exec.Command("go", "tool", "pprof", "-symbolize=none", "-traces", f.Name()).CombinedOutput() + if err != nil { + t.Fatalf("go tool pprof -traces: %v\n%s", err, traces) + } + if strings.Contains(string(traces), "memory._memset_neon") { + break + } + // Mitigate potential flakiness on CI runners + duration *= 2 + } + if !strings.Contains(string(traces), "memory._memset_neon") { + t.Fatal("CPU profile contains no _memset_neon samples after 5 attempts") + } + + want := strings.Join([]string{ + "github.com/apache/arrow-go/v18/arrow/memory._memset_neon", + "github.com/apache/arrow-go/v18/arrow/memory.memory_memset_neon", + "github.com/apache/arrow-go/v18/arrow/memory.Set", + "github.com/apache/arrow-go/v18/arrow/memory_test.TestNEONAssemblyTracebacks", + }, "\n") + + for trace := range strings.SplitSeq(string(traces), "-----------") { + if !strings.Contains(trace, "memory._memset_neon") { + continue + } + + // pprof adds whitespace and marks inlined functions, neither of which is + // part of the call stack. Remove both before requiring adjacent frames. + trace = strings.ReplaceAll(trace, "(inline)", "") + trace = strings.Join(strings.Fields(trace), "\n") + if !strings.Contains(trace, want) { + t.Errorf("invalid _memset_neon traceback:\n%s", trace) + } + } +} diff --git a/internal/utils/min_max_neon_arm64.s b/internal/utils/min_max_neon_arm64.s index 078971d1e..a037057e4 100644 --- a/internal/utils/min_max_neon_arm64.s +++ b/internal/utils/min_max_neon_arm64.s @@ -12,13 +12,7 @@ TEXT ·_int32_max_min_neon(SB), $0-32 MOVD minout+16(FP), R2 MOVD maxout+24(FP), R3 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100043f // cmp w1, #1 - WORD $0x910003fd // mov x29, sp BLT int32_early_exit WORD $0x71001c3f // cmp w1, #7 @@ -34,9 +28,6 @@ int32_early_exit: WORD $0x52b0000b // mov w11, #-2147483648 WORD $0xb900006b // str w11, [x3] WORD $0xb900004a // str w10, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET int32_neon: WORD $0x927d7109 // and x9, x8, #0xfffffff8 @@ -78,9 +69,6 @@ int32_scalar_loop: int32_done: WORD $0xb900006b // str w11, [x3] WORD $0xb900004a // str w10, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET // func _uint32_max_min_neon(values unsafe.Pointer, length int, minout, maxout unsafe.Pointer) @@ -91,13 +79,7 @@ TEXT ·_uint32_max_min_neon(SB), $0-32 MOVD minout+16(FP), R2 MOVD maxout+24(FP), R3 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100043f // cmp w1, #1 - WORD $0x910003fd // mov x29, sp BLT uint32_early_exit WORD $0x71001c3f // cmp w1, #7 @@ -113,9 +95,6 @@ uint32_early_exit: WORD $0x1280000b // mov w11, #-1 WORD $0xb900006a // str w10, [x3] WORD $0xb900004b // str w11, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET uint32_neon: WORD $0x927d7109 // and x9, x8, #0xfffffff8 @@ -157,9 +136,6 @@ uint32_scalar_loop: uint32_done: WORD $0xb900006a // str w10, [x3] WORD $0xb900004b // str w11, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET // func _int64_max_min_neon(values unsafe.Pointer, length int, minout, maxout unsafe.Pointer) @@ -170,13 +146,7 @@ TEXT ·_int64_max_min_neon(SB), $0-32 MOVD minout+16(FP), R2 MOVD maxout+24(FP), R3 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100043f // cmp w1, #1 - WORD $0x910003fd // mov x29, sp BLT int64_early_exit WORD $0x2a0103e8 // mov w8, w1 @@ -192,9 +162,6 @@ int64_early_exit: WORD $0xd2f0000b // mov x11, #-9223372036854775808 WORD $0xf900006b // str x11, [x3] WORD $0xf900004a // str x10, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET int64_neon: WORD $0x927e7509 // and x9, x8, #0xfffffffc @@ -246,9 +213,6 @@ int64_scalar_loop: int64_done: WORD $0xf900006b // str x11, [x3] WORD $0xf900004a // str x10, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET @@ -260,13 +224,7 @@ TEXT ·_uint64_max_min_neon(SB), $0-32 MOVD minout+16(FP), R2 MOVD maxout+24(FP), R3 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100043f // cmp w1, #1 - WORD $0x910003fd // mov x29, sp BLT uint64_early_exit WORD $0x71000c3f // cmp w1, #3 @@ -282,9 +240,6 @@ uint64_early_exit: WORD $0x9280000b // mov x11, #-1 WORD $0xf900006a // str x10, [x3] WORD $0xf900004b // str x11, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET uint64_neon: WORD $0x927e7509 // and x9, x8, #0xfffffffc @@ -336,7 +291,4 @@ uint64_scalar_loop: uint64_done: WORD $0xf900006a // str x10, [x3] WORD $0xf900004b // str x11, [x2] - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET diff --git a/parquet/internal/bmi/bitmap_neon_arm64.s b/parquet/internal/bmi/bitmap_neon_arm64.s index f711e4905..563548d71 100644 --- a/parquet/internal/bmi/bitmap_neon_arm64.s +++ b/parquet/internal/bmi/bitmap_neon_arm64.s @@ -8,13 +8,7 @@ TEXT ·_levels_to_bitmap_neon(SB), $0-32 MOVD numLevels+8(FP), R1 MOVD rhs+16(FP), R2 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! WORD $0x7100043f // cmp w1, #1 - WORD $0x910003fd // mov x29, sp BLT LBB1_3 WORD $0x71000c3f // cmp w1, #3 @@ -82,9 +76,6 @@ LBB1_7: WORD $0xaa080168 // orr x8, x11, x8 BNE LBB1_7 LBB1_8: - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP MOVD R8, res+24(FP) RET diff --git a/parquet/internal/utils/bit_packing_neon_arm64.s b/parquet/internal/utils/bit_packing_neon_arm64.s index b3a02f4cf..af1779f10 100644 --- a/parquet/internal/utils/bit_packing_neon_arm64.s +++ b/parquet/internal/utils/bit_packing_neon_arm64.s @@ -272,7 +272,7 @@ #define LCPI0_196 $0xfffffffcfffffffe #define LCPI0_200 $0xfffffffeffffffff -TEXT ·_unpack32_neon(SB), $0-40 +TEXT ·_unpack32_neon(SB), $496-40 MOVD in+0(FP), R0 MOVD out+8(FP), R1 @@ -281,19 +281,14 @@ TEXT ·_unpack32_neon(SB), $0-40 // LEAQ LCDATA1<>(SB), BP // %bb.0: - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9ba7bfd // stp x29, x30, [sp, #-96]! - WORD $0xd10643e9 // sub x9, sp, #400 - WORD $0xa9016ffc // stp x28, x27, [sp, #16] - WORD $0xa90267fa // stp x26, x25, [sp, #32] - WORD $0x910003fd // mov x29, sp - WORD $0xa9035ff8 // stp x24, x23, [sp, #48] - WORD $0xa90457f6 // stp x22, x21, [sp, #64] - WORD $0xa9054ff4 // stp x20, x19, [sp, #80] - WORD $0x927df13f // and sp, x9, #0xfffffffffffffff8 + // TODO: do not use x28, x27, or x18 since they are reserved. + // It is probably unnecessary to save the rest of these registers since + // the ABI doesn't have callee saved registers + WORD $0xa91a6ffc // stp x28, x27, [sp, #416] + WORD $0xa91b67fa // stp x26, x25, [sp, #432] + WORD $0xa91c5ff8 // stp x24, x23, [sp, #448] + WORD $0xa91d57f6 // stp x22, x21, [sp, #464] + WORD $0xa91e4ff4 // stp x20, x19, [sp, #480] WORD $0x11007c48 // add w8, w2, #31 WORD $0x7100005f // cmp w2, #0 WORD $0x1a82b108 // csel w8, w8, w2, lt @@ -6919,14 +6914,13 @@ LBB0_158: LBB0_156: WORD $0x531b6a60 // lsl w0, w19, #5 - WORD $0x910003bf // mov sp, x29 - WORD $0xa9454ff4 // ldp x20, x19, [sp, #80] - WORD $0xa94457f6 // ldp x22, x21, [sp, #64] - WORD $0xa9435ff8 // ldp x24, x23, [sp, #48] - WORD $0xa94267fa // ldp x26, x25, [sp, #32] - WORD $0xa9416ffc // ldp x28, x27, [sp, #16] - WORD $0xa8c67bfd // ldp x29, x30, [sp], #96 - // Put the stack pointer back where it was - ADD $16, RSP + WORD $0xa95e4ff4 // ldp x20, x19, [sp, #480] + WORD $0xa95d57f6 // ldp x22, x21, [sp, #464] + WORD $0xa95c5ff8 // ldp x24, x23, [sp, #448] + WORD $0xa95b67fa // ldp x26, x25, [sp, #432] + WORD $0xa95a6ffc // ldp x28, x27, [sp, #416] + // The raw compiler-generated body uses x30 as a scratch register. The + // Go-generated prologue saved the return address at the bottom of the frame. + MOVD 0(RSP), R30 MOVD R0, num+32(FP) RET diff --git a/parquet/internal/utils/unpack_bool_neon_arm64.s b/parquet/internal/utils/unpack_bool_neon_arm64.s index f4ea581ec..f8ef8bf63 100644 --- a/parquet/internal/utils/unpack_bool_neon_arm64.s +++ b/parquet/internal/utils/unpack_bool_neon_arm64.s @@ -11,13 +11,6 @@ TEXT ·_bytes_to_bools_neon(SB), $0-32 MOVD out+16(FP), R2 MOVD outlen+24(FP), R3 - // The Go ABI saves the frame pointer register one word below the - // caller's frame. Make room so we don't overwrite it. Needs to stay - // 16-byte aligned - SUB $16, RSP - WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]! - WORD $0x910003fd // mov x29, sp - WORD $0x7100043f // cmp w1, #1 BLT done @@ -145,7 +138,4 @@ scalar_next: JMP scalar_loop done: - WORD $0xa8c17bfd // ldp x29, x30, [sp], #16 - // Put the stack pointer back where it was - ADD $16, RSP RET