compiler: make TARGET_ATTRIBUTE and LIKELY/UNLIKELY capability checks consistent under clang-cl - #4729
Open
matevz-kovacic wants to merge 2 commits into
Open
Conversation
portability_macros.h enables DYNAMIC_BMI2 for clang-cl by testing __clang__ && __has_attribute(__target__). compiler.h then gates TARGET_ATTRIBUTE on __GNUC__, which clang-cl does not define, so BMI2_TARGET_ATTRIBUTE expands to nothing and the dispatched _bmi2 entry points are compiled without BMI2. Test the attribute directly with __has_attribute, keeping the __GNUC__ test as a fallback for compilers without __has_attribute. No change on any compiler that defines __GNUC__.
Same __GNUC__ gate as TARGET_ATTRIBUTE. clang-cl supports __builtin_expect, so test for it with __has_builtin and keep __GNUC__ as a fallback. Without this, every branch hint in the library is discarded under clang-cl.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TARGET_ATTRIBUTEandLIKELY/UNLIKELYinlib/common/compiler.haregated on
__GNUC__, which clang-cl does not define — although clang-clsupports both features. A clang-cl build therefore emits none of the
BMI1/BMI2 instructions the dynamic dispatch exists to select, and discards
every branch hint in the library.
This replaces the
__GNUC__test with a directcapability test (
__has_attribute/__has_builtin), keeping__GNUC__as a fallback so nothing changes for compilers without
__has_*.Missing under clang-cl: 683 instructions —
shrx377,shlx240,bzhi31,lzcnt27,andn8. Thebzhiare the ones #2689 introduced toreplace
BIT_masklookups. Why:DYNAMIC_BMI2is enabled under clang-cl,so dispatch is active, but
BMI2_TARGET_ATTRIBUTEexpands to nothing — the_bmi2entry points it calls were compiled without BMI2. Nothing ismiscompiled and nothing faults; the dispatch is paid for and returns
nothing.
Worth ~11% decompression speed using clang-cl on the machine measured below.
Commit 1:
TARGET_ATTRIBUTECommit 2:
LIKELY/UNLIKELYAre other macros affected the same way?
I audited every
__GNUC__gate inlib/. These two are the only ones withcode-generation consequences under clang-cl. The rest fall into three
groups:
_MSC_VERbranch, which clang-cl takes:FORCE_INLINE_ATTR(__forceinline),FORCE_NOINLINE(
__declspec(noinline)),PREFETCH_L1/L2(_mm_prefetch), and thecount-leading/trailing-zeros paths in
bits.h(_BitScanForward/_BitScanReverse).UNUSED_ATTR,MEM_STATIC,ERR_STATIC— warningsuppression and inline spelling, no codegen effect.
MEM_FORCE_MEMORY_ACCESSis leftundefined under clang-cl, selecting the
memcpypath rather than thepacked-struct one. Building with
-DMEM_FORCE_MEMORY_ACCESS=1producesbyte-identical objects (30/30), because clang lowers the constant-size
memcpyto the same load. Nothing to gain.ZSTD_ASM_SUPPORTEDinportability_macros.his also__GNUC__-gated anddoes evaluate to 0 for clang-cl — but that is the correct answer, since the
.Sfiles are GNU-syntax and clang-cl will not assemble them.Generated code
Instructions requiring the BMI2 target attribute, stock Release build of
the static library:
__GNUC__--target=x86_64-w64-mingw32clx64)The same clang binary already emits exactly 683 in GNU driver mode; this
patch brings it to the same 683 in MSVC driver mode. Same compiler, same
source, same flags — only the driver differed. MSVC's 0 is correct and
expected: it has no
__target__equivalent.These counts exclude
tzcnt, which is not gated by the target attribute —its encoding is backward-compatible with
bsf, so compilers emit it freely(clang-cl emits 565 either way, gcc 1622). Counting it would obscure the
effect rather than show it.
Effect on other compilers: none
Both macros already expand wherever
__GNUC__is defined, so the patchedheader should produce identical machine code there. Verified by diffing
per-object disassembly of baseline vs patched builds: gcc 16.1 30/30
objects byte-identical, clang
--target=x86_64-w64-mingw3232/32,MSVC
cl30/30. Only clang-cl changes.Correctness
The
_bmi2entry points now genuinely require BMI2, so this rests on theexisting runtime dispatch. Every one of the 683 instructions lands in a
function annotated
BMI2_TARGET_ATTRIBUTEorHUF_FAST_BMI2_ATTRS— noneleaked into a shared helper — so they are reached only through that
dispatch.
Checked by running it: a build with
ZSTD_cpuSupportsBmi2()forced to 0and
cctx->bmi2 = 0, simulating a CPU without BMI2, passes round-trip andcross-decode against a stock decoder while still containing all 683
instructions — present but never executed.
This is upstream's existing mechanism rather than new risk: a gcc build of
the unmodified baseline already places 705 such instructions inside those
same dispatched functions.
Benchmarks
One machine, one OS, one compiler — not a cross-platform claim.
i5-13400, Windows 11, clang-cl 22.1.8, baseline
v1.5.7(
f8745da6ff1ad1e7bab384bd1f9d742439278e99), identical flags both arms.zstd -b -d -i5 --single-thread, pinned to a single P-core at highpriority, arms interleaved within every repeat, median of 11 repeats,
aggregated over total corpus time. Levels 1, 3, 5, 8, 12, 16, 19. Corpora:
Silesia, plus a held-out set (enwik8, a source tarball, a large JSON file,
a tarball of native x86-64 binaries).
Decompression speed:
Every level improved, in all three runs. For commits 1+2, at every level
the slowest of the 11 patched samples was still faster than the fastest of
the 11 baseline samples — the two sets never overlap. Re-running the
Silesia measurement in a separate timing window reproduced it to within
0.24 percentage points.
Compression ratio: unchanged, and structurally so — the patch changes
only which instructions compute the same values, not any encoding
decision. Confirmed anyway: every corpus file recompressed with both
binaries at every level, 112 / 112 byte-identical by SHA-256.
Compression speed: measured, not assumed. 217 of the 683 instructions
land on the compression side (
huf_compress+182,zstd_compress_sequences+35), and atarget-attributed function can nolonger be inlined into a non-attributed caller — so a compression effect in
either direction is possible in principle. Measured: +0.40% overall. I
claim no improvement; run-to-run variation at the high levels reaches ~20%
on this machine, so the honest statement is only that no regression was
detectable. I did not separately isolate the inlining cost.
Notes
Commit 2 is independent and can be dropped if you would rather consider
branch-hint changes separately. It adds no annotations — those are already
in the tree (#2689 added several to
ZSTD_decodeSequence) and this onlystops one driver mode from discarding them. Its contribution is measured
separately above because
compiler.hasks that these macros earn theirplace.
Not tested: 32-bit clang-cl; a genuinely pre-BMI2 CPU (that path was
exercised by forcing dispatch off, not on such hardware); the cost of the
attribute's inlining restriction in isolation.
Tested:
tests/fuzzerandtests/zstreamtest(-T90s -s1) built with clang-clfrom both the baseline and the patched tree — both exit 0 on both arms.
These are the ones that exercise the changed code.
Silesia, the held-out set, and a generated adversarial set.
tests/playTests.shpasses on Linux (WSL/Ubuntu, gcc 13.3): exit 0, 55sections, with only the usual conditional skip of the large-data tests.
Worth noting what that does and does not show — on gcc this patch is a
byte-identical no-op, so it confirms the header change breaks nothing in a
POSIX build rather than exercising the new code generation.