Description
I was reviewing #239 and checked whether we had any test coverage for the NEON FIR kernel in internal/resample/silk/iir_fir_arm64.s. As far as I can tell, we don't, and right now CI doesn't even assemble that file.
The build tag is arm64 && go1.27 && !purego, while the test matrix in test.yaml is still on Go 1.24/1.25. test macos does run on arm64 (macos-26), but because of the Go version it takes the generic path. Conformance and encoder-quality use 1.27, but those run on ubuntu-latest, so they're amd64.
So at the moment this kernel isn't being assembled or tested anywhere in CI.
Nothing is broken right now. The part that concerns me is what happens when the Go version matrix gets bumped. It has the auto-update/supported-go-version-list marker, so once that reaches 1.27 this code starts being used by macOS CI and by arm64 users building with a current toolchain, without any parity test behind it.
I think it would be better to land the test before that version bump rather than after it. I'm happy to do that first.
I also went through the assembly manually and it looks correct to me. The phase selection and reversed tap layout line up with the generic implementation, SRSHR #15 matches silk_RSHIFT_ROUND, saturation happens after the sum, and the wrapper checks the spans before passing raw pointers into assembly.
One thing I checked while doing that was whether the int32 accumulator could overflow. The largest sum of absolute tap values for a phase is 50045, which puts the worst-case full-scale int16 input at about 1.64e9. That's still below the int32 limit of 2.147e9. So the reassociated NEON reduction should be exact here; we're not depending on int32 wraparound behavior.
I have a parity test locally that compares the NEON kernel against resamplerPrivateIIRFIRInterpolateGeneric. It covers all 12 phases, saturation, aliased output, and the wrapper fallback paths.
I also tried deliberately breaking the assembly — changing the rounding shift, dropping VSMLAL2, using the wrong phase count, and changing the strides — and the test fails in each case.
It passes on native arm64 with Go 1.27 on both ubuntu-24.04-arm and macos-26. The macOS runner is the same one test-macos already uses, so this shouldn't need any new CI infrastructure:
https://github.com/thomas-vilte/opus/actions/runs/33789089378
Happy to open a PR with the test. The Go version bump itself looks like a separate .goassets change.
I also found an unrelated arm64 issue while running this: go test ./internal/celt fails without -race in TestExpRotation1BlockOfFour, with last-bit float32 differences from FMA contraction. I reproduced it on both Linux and macOS arm64.
CI currently misses that because test-macos runs go test -race ./.... I'll open a separate issue for it.
@zshang-oai one thing I wasn't able to reproduce is the benchmark table. Could you rerun it on the M4 Max against the merged commit, once with -tags=purego and once with NEON enabled?
The numbers in the PR include both the portable rewrites and the assembly kernel, so I'd like to see how much the NEON path itself is buying us.
Follow-up to #239.
Description
I was reviewing #239 and checked whether we had any test coverage for the NEON FIR kernel in
internal/resample/silk/iir_fir_arm64.s. As far as I can tell, we don't, and right now CI doesn't even assemble that file.The build tag is
arm64 && go1.27 && !purego, while the test matrix intest.yamlis still on Go 1.24/1.25.test macosdoes run on arm64 (macos-26), but because of the Go version it takes the generic path. Conformance and encoder-quality use 1.27, but those run onubuntu-latest, so they're amd64.So at the moment this kernel isn't being assembled or tested anywhere in CI.
Nothing is broken right now. The part that concerns me is what happens when the Go version matrix gets bumped. It has the
auto-update/supported-go-version-listmarker, so once that reaches 1.27 this code starts being used by macOS CI and by arm64 users building with a current toolchain, without any parity test behind it.I think it would be better to land the test before that version bump rather than after it. I'm happy to do that first.
I also went through the assembly manually and it looks correct to me. The phase selection and reversed tap layout line up with the generic implementation,
SRSHR #15matchessilk_RSHIFT_ROUND, saturation happens after the sum, and the wrapper checks the spans before passing raw pointers into assembly.One thing I checked while doing that was whether the int32 accumulator could overflow. The largest sum of absolute tap values for a phase is 50045, which puts the worst-case full-scale int16 input at about 1.64e9. That's still below the int32 limit of 2.147e9. So the reassociated NEON reduction should be exact here; we're not depending on int32 wraparound behavior.
I have a parity test locally that compares the NEON kernel against
resamplerPrivateIIRFIRInterpolateGeneric. It covers all 12 phases, saturation, aliased output, and the wrapper fallback paths.I also tried deliberately breaking the assembly — changing the rounding shift, dropping
VSMLAL2, using the wrong phase count, and changing the strides — and the test fails in each case.It passes on native arm64 with Go 1.27 on both
ubuntu-24.04-armandmacos-26. The macOS runner is the same onetest-macosalready uses, so this shouldn't need any new CI infrastructure:https://github.com/thomas-vilte/opus/actions/runs/33789089378
Happy to open a PR with the test. The Go version bump itself looks like a separate
.goassetschange.I also found an unrelated arm64 issue while running this:
go test ./internal/celtfails without-raceinTestExpRotation1BlockOfFour, with last-bit float32 differences from FMA contraction. I reproduced it on both Linux and macOS arm64.CI currently misses that because
test-macosrunsgo test -race ./.... I'll open a separate issue for it.@zshang-oai one thing I wasn't able to reproduce is the benchmark table. Could you rerun it on the M4 Max against the merged commit, once with
-tags=puregoand once with NEON enabled?The numbers in the PR include both the portable rewrites and the assembly kernel, so I'd like to see how much the NEON path itself is buying us.
Follow-up to #239.