Skip to content

Fix Windows MSVC Debug build of XNNPACK and clang-cl kernel gating - #22163

Open
SS-JIA wants to merge 1 commit into
mainfrom
fix-windows-msvc-xnnpack-preprocessor
Open

Fix Windows MSVC Debug build of XNNPACK and clang-cl kernel gating#22163
SS-JIA wants to merge 1 commit into
mainfrom
fix-windows-msvc-xnnpack-preprocessor

Conversation

@SS-JIA

@SS-JIA SS-JIA commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes #22084.

The bug

Any Debug preset with XNNPACK enabled fails to compile XNNPACK/src/subgraph.c under the MSVC toolset:

subgraph.c(2663,3): error C2143: syntax error: missing ')' before 'string'
subgraph.c(2663,3): error C2059: syntax error: ')'
subgraph.c(2918,5): error C2143: syntax error: missing ')' before 'string'
subgraph.c(3500,5): error C2143: syntax error: missing ')' before 'string'

XNNPACK nests two variadic macros. XNN_RETURN_IF_ERROR (src/xnnpack/internal.h) expands to xnn_log_error("" __VA_ARGS__), and xnn_log_error (src/xnnpack/log.h) is itself variadic and concatenates a location suffix onto its first parameter:

#define xnn_log_error(format, ...) \
  xnn_log_error_(format " (%s, %s:%i)", ##__VA_ARGS__, __FUNCTION__, __FILE__, __LINE__)

MSVC's default legacy preprocessor forwards the outer __VA_ARGS__ into the inner macro as a single un-split argument. format therefore binds to the whole blob and the suffix is glued onto the last argument instead of the format string, yielding node_id " (%s, %s:%i)". Only the three call sites in subgraph.c that pass a format string plus format arguments are affected, which is exactly lines 2663, 2918 and 3500 at the currently pinned XNNPACK revision.

/Zc:preprocessor selects MSVC's conformant preprocessor and resolves it. The guard keys off CMAKE_C_COMPILER_ID rather than the MSVC variable because MSVC is also true under clang-cl, which already conforms.

Why CI is green today

XNN_LOG_LEVEL is 5 only for $<CONFIG:Debug> and 0 otherwise, so in Release xnn_log_error expands to nothing and the broken expansion never happens. The existing windows-msvc job builds Release only, leaving Debug uncovered. A cheap regression guard would be adding -DEXECUTORCH_XNNPACK_LOG_LEVEL=5 to .ci/scripts/setup-windows-msvc.ps1, which exercises the path without a second full build; I left that out of this PR to keep it reviewable, happy to add it if wanted.

Second, related fix

tools/cmake/preset/llm.cmake disabled the quantized and LLM kernels on Windows whenever the MSVC variable was set. CMake also sets MSVC for clang-cl, so those kernels were silently dropped from every Windows clang-cl build while the accompanying warning advised the user to switch to -T ClangCL in order to get them. That contradicts the stated intent of #15719 ("Only block when building with MSVC"), so the check now uses CMAKE_CXX_COMPILER_ID. The pybind preset already enables the same kernels and is built with -T ClangCL in Windows CI, so this path is covered.

Testing

On Windows with VS 2022 (MSVC 19.43):

  • xnnpack-subgraph in Debug under the default MSVC toolset fails with the errors above before the change and builds cleanly after it.
  • All 96 XNNPACK logic-layer sources compiled with XNN_LOG_LEVEL=5, with and without /Zc:preprocessor, confirming the flag introduces no new diagnostics.
  • cmake --preset llm-debug still disables the kernels and warns under MSVC, and now enables them under -T ClangCL, where custom_ops, quantized_kernels and quantized_ops_lib all build clean.

cmake-format and cmakelint pass on both files.

Follow-up, not in this PR

make parakeet-vulkan builds the ExecuTorch core with llm-debug-vulkan but the runner with a preset pinned to Release. On MSVC that is a /MDd vs /MD CRT mismatch, so the reporter is likely to hit LNK2038 next. Every other backend pairs like with like (parakeet-cuda uses llm-release-cuda). The fix is presumably an llm-release-vulkan preset mirroring llm-release-cuda, but that changes behaviour for existing Linux users of the target so it belongs on its own.

This PR was authored with Claude Code.

🤖 Generated with Claude Code

@SS-JIA @claude
Fix Windows MSVC Debug build of XNNPACK and clang-cl kernel gating
77bcb0e 
Summary:

Building any Debug preset with XNNPACK enabled under the MSVC toolset fails to
compile backends/xnnpack/third-party/XNNPACK/src/subgraph.c. XNNPACK nests two
variadic macros: XNN_RETURN_IF_ERROR expands to xnn_log_error, which in turn
concatenates a location suffix onto its first parameter. MSVC's default legacy
preprocessor forwards the outer __VA_ARGS__ into the inner macro as a single
un-split argument, so the suffix is glued onto the last format argument rather
than the format string, producing "syntax error: missing ')' before 'string'".
Only the three call sites that pass a format string plus format arguments are
affected. Release builds escape it because XNN_LOG_LEVEL is 0 outside of Debug,
which makes xnn_log_error expand to nothing, and that is why the Release-only
windows-msvc CI job never caught this. Enable /Zc:preprocessor for XNNPACK when
the compiler is genuine MSVC.

The same investigation turned up a second problem. The llm preset disables the
quantized and LLM kernels on Windows whenever the MSVC variable is set, but
CMake also sets MSVC for clang-cl, so those kernels were being dropped from
clang-cl builds while the accompanying warning told the user to switch to
-T ClangCL in order to get them. That contradicts the stated intent of #15719,
so the check now keys off CMAKE_CXX_COMPILER_ID instead.

Fixes #22084.

Test Plan:

On Windows with VS 2022 (MSVC 19.43), configured with the default MSVC toolset
and built the xnnpack-subgraph target in Debug. Before the change the build
fails with C2143/C2059 at subgraph.c lines 2663, 2918 and 3500, matching the
issue report exactly; after the change it succeeds and produces
xnnpack-subgraph.lib. Separately compiled all 96 XNNPACK logic-layer sources
with XNN_LOG_LEVEL=5 both with and without /Zc:preprocessor to confirm the flag
introduces no new diagnostics.

Verified that cmake --preset llm-debug still disables the quantized and LLM
kernels and warns under MSVC, and now enables them under -T ClangCL, where
custom_ops, quantized_kernels and quantized_ops_lib all build clean.

Reviewers:

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pytorch-bot

pytorch-bot Bot commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22163

Note: Links to docs will display an error until the docs builds have been completed.

❌ 19 New Failures, 4 Unclassified Failures

As of commit 77bcb0e with merge base 69a3979 (image):

NEW FAILURES - The following jobs have failed:

UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@digantdesai digantdesai digantdesai approved these changes
@larryliu0820 larryliu0820 Awaiting requested review from larryliu0820 larryliu0820 is a code owner
@kirklandsign kirklandsign Awaiting requested review from kirklandsign kirklandsign is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Windows] CMake preset llm-debug-vulkan selects MSVC instead of Clang, causing XNNPACK compilation failures

2 participants

@SS-JIA @digantdesai