Skip to content

fix(sycl): clean integer_adm_sycl warnings and scope the SYCL tidy lane to changed files - #1281

Merged
lusoris merged 5 commits into
masterfrom
fix/sycl-adm-tidy-debt
Sep 5, 2026
Merged

fix(sycl): clean integer_adm_sycl warnings and scope the SYCL tidy lane to changed files#1281
lusoris merged 5 commits into
masterfrom
fix/sycl-adm-tidy-debt

Conversation

@lusoris

@lusoris lusoris commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Clang-Tidy SYCL (Changed Files, Advisory) reddened #1223 — a PR whose only SYCL file is float_vif_sycl.cpp — with warnings from core/src/feature/sycl/integer_adm_sycl.cpp, a file it never touched. Two separate problems, two separate commits.

(A) Real defects in integer_adm_sycl.cpp

  • Five designated-initialiser lists out of declaration order. ISO C++ requires declaration order; this is ill-formed, and MSVC rejects it outright rather than warning.
  • A doubled const specifier (const int32_t const const *p). Rewritten as const int32_t *const p; the pointee stays const, and since the arrays are never reassigned after init the pointer can be too — which is also why the misc-const-correctness NOLINT that claimed the variable "is mutated" could go.

Fixed in code. No NOLINT added.

(B) The lane's scoping bug — why an untouched file failed an unrelated PR

The job ran meson compile -C build-sycl (the whole tree, under icpx) before clang-tidy, so any translation unit's compiler warning failed the job regardless of the PR's diff. The lane's name promised "changed files"; the build step didn't. It now builds only include/vcs_version.h — the one generated header clang-tidy needs — and scripts/ci/clang-tidy-sycl.sh pins -std=c++20 for stock clang-tidy and drops the -D__SYCL_DEVICE_ONLY__=0 guard.

What was taken back out — and why

The agent that made the cleanup also changed ks = 17 - clz to (17 - clz > 0) ? (17 - clz) : 1 at two sites. That is a numeric change to an ADM kernel, not a lint fix: the CPU reference has no such clamp, so it would move SYCL off CPU parity for any input with clz > 16, and no justification was recorded. Reverted in the third commit. Whether a negative shift is actually reachable there is a fair question — it is now an open row in docs/state.md (T-SYCL-ADM-NEGATIVE-SHIFT-REACHABILITY-2026-09-04) that needs a CPU-parity analysis and a cross-backend diff of its own.

Type

  • fix — bug fix
  • build / ci — tooling / infra
  • sycl — backend-specific

Checklist

  • Commits follow Conventional Commits.
  • make format && make lint is green locally.
  • Unit tests pass — full SYCL build under icpx (oneAPI, -Denable_sycl=true) completes with SYCL_BUILD_RC=0; the CPU fast suite is unaffected (no CPU file changed).
  • If I touched any SIMD/GPU code path, I ran /cross-backend-diff and the worst ULP is ≤ 2. — The only numeric change in the branch's history (the clamp) was reverted; the surviving edits are declaration-order and const-qualifier changes that cannot alter codegen semantics. No ULP delta is possible from what ships.
  • If I touched a feature extractor with SIMD/GPU twins, I either updated every twin or listed the gap. — Only the SYCL twin had the warnings; CPU/CUDA/HIP twins were checked and are clean.
  • If I added a new .c / .cpp / .cu / .h / .hpp, it has the license header. — n/a.
  • If this is a breaking change, the commit message uses !. — n/a.
  • If this PR adds an ADR, the row lives in docs/adr/_index_fragments/. — n/a; no design decision.
  • Docs — no docs needed: initialiser-order and const-qualifier cleanup inside integer_adm_sycl.cpp plus CI-lane scoping; no user-visible behaviour, flag, or output changes, so no docs/backends/sycl/ page is affected.

Bug-status hygiene (ADR-0165)

Netflix golden-data gate (ADR-0024)

  • I did not modify any assertAlmostEqual(...) score in the Netflix golden Python tests.

Cross-backend numerical results

None expected and none possible from the shipped diff: initialiser order and const-ness do not change generated code. The one change that would have moved numbers was reverted before this PR was opened.

Deep-dive deliverables (ADR-0108)

  • Research digest — no digest needed: trivial. The warnings are what icpx printed; the scoping bug is one line of workflow YAML.
  • Decision matrix — no alternatives: only-one-way fix. Declaration order is mandated by the standard; the lane must build less, not analyse less.
  • AGENTS.md invariant note — no rebase-sensitive invariants: every touched path is fork-added with no upstream counterpart. The don't-clamp and don't-full-compile invariants are in docs/rebase-notes.md.
  • Reproducer / smoke-test command — below.
  • CHANGELOG fragmentchangelog.d/fixed/sycl-adm-tidy-scoping.md.
  • Rebase notedocs/rebase-notes.md.

Reproducer

# (A) the warnings, on master, with the same toolchain CI uses
source /opt/intel/oneapi/setvars.sh
meson setup core core/build-sycl -Denable_sycl=true -Denable_cuda=false
ninja -C core/build-sycl 2>&1 | grep -E 'integer_adm_sycl.cpp.*(field designators|duplicate .const.)'
# -> five "ISO C++ requires field designators to be specified in declaration order"
#    and two "duplicate 'const' declaration specifier" on master; none on this branch

# (B) the scoping bug: #1223 touched only float_vif_sycl.cpp, yet the lane failed on integer_adm_sycl.cpp
gh pr diff 1223 --name-only | grep sycl          # -> core/src/feature/sycl/float_vif_sycl.cpp only

Verification performed

Same object, same icpx, same flags — only the source file swapped:

integer_adm_sycl.cpp version designator-order warnings duplicate-const warnings total warnings
origin/master 15 12 28
this branch 0 0 1

The one remaining warning is icpx: warning: argument unused during compilation: '-Xs -device …' — the build system's device list, reported on every SYCL TU regardless of source, and visible in CI's own logs before this change. Out of scope here.

Check Result
Full SYCL build (ninja -C core/build-sycl) SYCL_BUILD_RC=0, 210/210 targets
Object rebuild, fixed source 0 of either warning class
Object rebuild, master's source (negative control) 15 + 12
git checkout -- integer_adm_sycl.cpp after the control clean
pre-commit on every touched file clean
concat-changelog-fragments.sh --check exit 0

Known follow-ups

  • T-SYCL-ADM-NEGATIVE-SHIFT-REACHABILITY-2026-09-04 (open): is clz > 17 reachable at integer_adm_sycl.cpp:705,1032, and what does the CPU integer ADM do there? Needs parity analysis before any change.
  • The -D__SYCL_DEVICE_ONLY__=0 removal in the wrapper is verified only by this PR's own advisory-lane run; if that lane shows stock clang-tidy choking on device-only intrinsics, the guard should come back.

🤖 Generated with Claude Code

Lusoris and others added 4 commits September 4, 2026 23:01
The tidy cleanup changed `ks = 17 - clz` to `(17 - clz > 0) ? (17 - clz) : 1`
at both sites in integer_adm_sycl.cpp. That is a numeric change to an ADM
kernel, not a lint fix: the CPU reference carries no such clamp, so the
change would move SYCL away from CPU parity for any input where clz > 16,
and no justification was recorded. Reverted. If a negative shift is
actually reachable here it is a real finding, but it needs a CPU-parity
analysis and a cross-backend diff of its own, not a ride-along in a
warning cleanup. Tracked in docs/state.md as an open row.

Also renders the changelog fragment the cleanup added.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… question

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@lusoris
lusoris marked this pull request as ready for review September 4, 2026 21:01
@lusoris
lusoris force-pushed the fix/sycl-adm-tidy-debt branch from a249e8b to 617a24c Compare September 4, 2026 21:01
@lusoris
lusoris merged commit ed0af7a into master Sep 5, 2026
73 of 79 checks passed
@lusoris
lusoris deleted the fix/sycl-adm-tidy-debt branch September 5, 2026 01:04
@lusoris lusoris added the type:bug Something isn't working label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant