Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbc4b8d3d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Hardware-specific CPUID/XCR0 dispatch behavior warrants final human validation on representative APX systems.
Review effort: Balanced
Findings: None
What changed in this PR
Separates APX usability from NVL/DMR ISA detection for safe host selection and runtime dispatch.
Changes:
- Adds independent AMX/APX capability checks and dispatch guards.
- Supports APX-disabled NVL/DMR targets while preserving DMR’s AMX requirement.
- Adds regression tests and documentation.
| File | Description |
|---|---|
src/isa.h |
Implements capability probing and host fallback logic. |
src/ispc.cpp |
Applies capability checks to host selection. |
src/ispc.h |
Adds full-APX-disable detection. |
src/module.cpp |
Generates APX-aware dispatch predicates. |
src/builtins.cpp |
Links the APX probe. |
src/builtins-decl.h |
Declares the APX builtin name. |
src/builtins-decl.cpp |
Defines the APX builtin name. |
builtins/dispatch.c |
Exposes runtime APX probing. |
tests/lit-tests/host-isa-selection.cpp |
Tests host fallback decisions. |
tests/lit-tests/dispatch-nvl-amx-guard.ispc |
Verifies non-APX dispatch output. |
tests/lit-tests/dispatch-nvl-amx-guard_llvm22_plus.ispc |
Tests NVL/DMR capability guards. |
docs/ispc.rst |
Documents full versus partial APX disabling. |
docs/ReleaseNotes.txt |
Records the dispatch behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The requirements slightly changed for this request. I'll be reviewing it next week. |
Summary
--opt=disable-apx, while retaining other requirements, including AMX for DMRRoot cause
Some older applications can crash when APX is enabled because they use previously unsupported opcodes that APX now supports. APX may therefore be disabled in the OS for compatibility while AVX10.2 remains available.
ISPC treated APX CPU support as part of NVL/DMR ISA detection without independently checking the required APX OS state. This could select code requiring APX when APX was not usable, or exclude otherwise compatible AVX10.2 targets when APX code generation was fully disabled. Host selection also needed to check AMX usability for targets requiring it.
The fix separates ISA detection from these usability checks.
--opt=disable-apxdisables APX instruction generation, not APX in the OS; disabling only some APX subfeatures still requires usable APX.Related Issue
Checklist
clang-format(20.1.8, matching CI)