Skip to content

Build and test the SILK NEON kernel in CI - #255

Closed
hashirmuzaffar wants to merge 1 commit into
pion:mainfrom
hashirmuzaffar:neon-ci-coverage
Closed

hashirmuzaffar wants to merge 1 commit into
pion:mainfrom
hashirmuzaffar:neon-ci-coverage

Conversation

@hashirmuzaffar

Copy link
Copy Markdown

Fixes #240.

The parity test from #243 exists, but nothing in CI ever assembles the kernel it covers. The build tag is arm64 && go1.27 && !purego; the matrix in test.yaml is on the supported release list, so test-macos is arm64 on an older Go and the newer-Go jobs are amd64. The file is never built and the test never runs.

I did not change test.yaml. It carries a DO NOT EDIT header and is synced from pion/.goassets, so an edit here would be reverted, and raising the shared matrix would move every pion repository onto a newer Go to cover one kernel in this one. conformance.yml, encoder-quality.yml and plc-quality.yml are already repo-local, so this follows that pattern.

The job does three things:

  1. Asserts the kernel is in the build before testing anything. This is the part that matters — without it, a build tag that stopped matching would leave the job green while compiling nothing, which is exactly the situation the issue describes. It inspects go list output for iir_fir_arm64.s and fails with an error annotation if it is absent.
  2. Runs the parity test under -race.
  3. Runs the package again under -tags purego, so the generic path stays covered on the same runner.

Verified locally on arm64 with Go 1.27:

go list ... ./internal/resample/silk   ->  iir_fir.go iir_fir_arm64.go iir_fir_arm64.s
TestIIRFIRInterpolateNEONMatchesGeneric      PASS
TestIIRFIRInterpolateNEONCoversEveryPhase    PASS
TestIIRFIRInterpolateAccumulatorBound        PASS

go list -tags purego ...                ->  iir_fir.go iir_fir_generic.go
go test -tags purego ...                     ok

The guard behaves as intended under purego: the kernel is correctly absent, which is what would fail the job if the tag ever stopped matching in the NEON configuration.

If you would rather take the matrix change discussed in the issue thread, 1.24/1.27 on test-macos, that belongs in .goassets and I am happy to send it there instead.

The NEON FIR kernel is guarded by arm64 && go1.27 && !purego, because the
assembler only learned those instructions in Go 1.27. The shared matrix in
test.yaml tracks the supported release list, which is behind that, so nothing
in CI assembles the file: the macOS runner is arm64 but on an older Go, and
the jobs on a newer Go are amd64. The parity test added in pion#243 therefore
never runs.

This adds a repo-local job on an arm64 runner with Go 1.27. It is kept out of
test.yaml because that file is synced from pion/.goassets, so a change there
would be overwritten, and raising the shared matrix would move every pion
repository onto a newer Go for one kernel in this one.

The job first asserts that iir_fir_arm64.s is genuinely in the build for this
configuration. Without that check a build tag which stopped matching would
leave the job green while compiling nothing, which is the failure this issue
is about. It then runs the parity test, and runs the package again under
purego so the generic path stays covered.

Fixes pion#240
@JoTurk

JoTurk commented Sep 17, 2026

Copy link
Copy Markdown
Member

We manage the ci in one repo in goassets, soon we'll change our test matrix to include 1.27 in the next ci update

@JoTurk JoTurk closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SILK NEON resampler kernel has no parity test, and no CI job builds it

2 participants