Fix UB signed shifts in SignedDigitDecompose and accelerate gadget decomposition - #1238
Merged
Conversation
…DigitDecompose
The carry (d0 - r0) >> gBits equals (d0 + gHalf) >> gBits since the digit r0 lies in [-gHalf, gHalf). The new form removes the extracted digit from the loop-carried dependency chain (4 -> 2 cycles per digit) and drops the now-unneeded digit extraction ahead of the loop. Output is bit-identical.
Process one gadget digit at a time across the whole polynomial, carrying the running quotient per coefficient in a scratch array. The inner loops become unit-stride and auto-vectorize (AVX2/AVX-512) when built with native optimizations; output is bit-identical to the previous order.
…ecompose Bias each coefficient once by H (gHalf in every digit position); every balanced digit then reads out as an independent unsigned window ((w >> k*gBits) & gMask) - gHalf. This removes the per-digit carry chain and all signed-shift arithmetic, drops the SignExtend helper, and speeds up the decomposition a further 1.2-1.8x over the digit-major form (up to 10.6x total vs the original). Output is bit-identical. Requires digitsG*gBits < MaxBits, satisfied by all supported parameter sets.
yspolyakov
approved these changes
Aug 5, 2026
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
Issue #1226 reported deterministic UBSan failures (
left shift of negative value,left shift ... cannot be represented) in both overloads ofRingGSWAccumulator::SignedDigitDecompose, reached by any ordinaryEvalBinGatecall. The offending expressions were the classic
(d << k) >> ksign-extensionidiom, deliberately written to compile to the hardware sign-extender (
shl/sar)— this is a bottleneck path, so the reconstruction-style rewrite suggested in the
issue would have traded correctness-on-paper for real overhead.
This PR fixes the UB with zero performance cost, and then — prompted by what we
found while validating the fix — restructures the decomposition so that the final
code contains no sign extension and no signed shifts at all, making the original
concern moot by construction, while running substantially faster.
The fix (f327504)
Perform the left shift in the unsigned type and shift back arithmetically:
Unsigned left shift is well-defined; the value-preserving conversion back and the
arithmetic right shift are implementation-defined (not UB) and already relied on
elsewhere in the function. Generated code is unchanged — byte-identical bodies with
clang 18 and an identical instruction mix (same
shl/sarpairs) with gcc 13 at-O3— and the reporter's TOY/LMKCDEY repro runs UBSan-clean.The optimization (1c4674a, 4dc0ed0, 4667738)
Verifying the fix exposed that the surrounding loop, not the shift idiom, was the
real cost. A hardware-adder analogy explains the three steps:
The original loop is a ripple-carry structure. Each digit is extracted and
then fed back into the carry:
r = signext(d); d = (d - r) >> gBits. Like aripple adder, stage k+1 cannot start until stage k completes — a 4-op
dependency chain per digit.
Decouple the carry from the digit (1c4674a). Since
r ≡ d (mod baseG)withr ∈ [-baseG/2, baseG/2), the carry has a closed form that never touches thedigit:
d = (d + baseG/2) >> gBits. The chain halves and digit extraction movesoff the critical path — analogous to separating an adder's carry path from its
sum logic.
Vectorize across coefficients (4dc0ed0). The N polynomial coefficients are
independent chains, so interchange the loops to digit-major order and let SIMD
process 4/8/16 coefficients per instruction — an array of cheap adders instead of
one fast one.
Full carry-lookahead (4667738). Just as a lookahead adder computes every
carry as a closed-form function of the primary inputs, biasing each coefficient
once by
H(=baseG/2in every digit position) makes every balanced digit anindependent unsigned window of the biased value:
The carry chain disappears entirely, along with the
SignExtendhelper and everysigned shift in the function. Requires
digitsG*gBits < MaxBits, which allsupported parameter sets satisfy with ample margin (≤53 of 64 bits; ≤31 of 32 for
NATIVE_SIZE=32builds).Correctness
Outputs are bit-identical to the original code at every step (exhaustive
differential harness over TOY/STD128-shaped parameters plus edge values
0, ±1, Q−1, Q/2±1). The full binfhe unit suite passes in all 8 validated buildconfigurations — {clang 18, gcc 14} × {
WITH_NATIVEOPTON/OFF} ×{
NATIVE_SIZE64/32} — and the issue's reproducer is UBSan-clean from the firstcommit onward.
Performance
Measured on a dual Xeon Platinum 8360Y (Ice Lake, AVX-512), min of 4 repetitions,
single-thread pinned; 36-thread runs show the same or better ratios. Speedups are
dev → this PR:
SignedDigitDecompose(native-opt ON)SignedDigitDecompose(native-opt OFF)EvalBinGateend-to-endNo regressions in any of the 32 measured configuration/thread-count cells.
Fixes #1226.