Branchless wide-integer comparisons and sign extension#109474
Conversation
…tor_greater
The generic limb-by-limb comparison loop in wide::integer::_impl::operator_less
and operator_greater compiles to data-dependent branches: an early-exit branch
per limb plus a sign pre-check branch for signed types. In columnar comparison
loops (NumComparisonImpl in FunctionsComparison.h) these branches prevent
vectorization and mispredict heavily on real data. The scalar branchy kernel is
visible in release builds: the compiled
NumComparisonImpl<UInt128, UInt128, LessOp>::vectorVector processes one element
per iteration with two data-dependent branches, while EqualsOp gets a proper
SIMD loop.
For 128-bit same-type comparisons on little-endian platforms, compare via
native (unsigned) __int128 instead: std::bit_cast to the native type and a
single comparison, which compiles to branchless cmp/sbb. Two's complement
order of __int128 matches the existing semantics exactly (sign pre-check plus
unsigned lexicographic limb comparison).
Standalone kernel benchmark of the columnar less-than loop (clang 21,
-march=x86-64-v3, 64k-element blocks, GB/s of processed data, higher is
better):
data shape before after
UInt128, low 64 bits only 46.9 56.1
UInt128, fully random 55.7 56.9
UInt128, 50% equal high limb 8.4 57.0
Int128, low 64 bits only 31.2 51.8
Int128, fully random 8.0 51.8
Int128, 50% equal high limb 8.1 47.9
The worst cases (unpredictable limb-equality or sign patterns) improve up to
6.8x, and no data shape regresses. 256-bit comparisons are left unchanged: the
branchy early-exit loop wins on predictable data there and the branchless
alternative regresses it, so it is not a clear improvement.
Result equivalence with the previous implementation was verified by an
exhaustive check of all comparison operators (<, >, <=, >=, ==, !=) for
UInt128/Int128/UInt256/Int256 over boundary values (0, 1, -1, min, max, sign
bit, 2^64 boundaries) and 12 million structured-random pairs (equal limbs,
off-by-one limbs, negated pairs, equal values), plus mixed-operand shapes and
constexpr evaluation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…f test XML Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [8d36d93] Summary: ✅
AI ReviewSummaryThis PR makes Final VerdictStatus: ✅ Approve |
The review asked for a focused regression test for the new native __int128 fast path in wide::integer::_impl::operator_less and operator_greater. Add a gtest that checks all six comparison operators for Int128 and UInt128 over boundary values (min/max, sign-bit transitions, 2^64 limb boundaries, equal-high-limb pairs) against the 256-bit comparison as an independent oracle: 256-bit comparisons use the generic limb-wise path that the fast path replaced for 128 bits. Also check constexpr evaluation of the fast path via static_assert and a few hand-picked direct assertions that do not depend on the oracle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Algunenano I suppose those are flaky tests |
Yes, it's being fixed. I'll handle it, don't worry about it |
Algunenano
left a comment
There was a problem hiding this comment.
The perf tests show regressions on some cases but a big speedup on the sort case (ORDER BY i1 -60%). An odd case is SELECT count() FROM cmp128 WHERE i1 < i2 where the CI sees a regression on x86, and a speed up on ARM.
PS: 04001_join_reorder_through_expression is fixed. Merging master or just pushing a new commit will fix it.
There was a problem hiding this comment.
The perf tests show regressions on the vector/vector kernels (i1 < i2 +15%, i1 >= i2 +17%) but a big speedup on the sort case (ORDER BY i1 -60%). Looks like might have regressed the v4 code path, which was vectorized before.
Analysis (it's LLM-generated, but I'm guiding the analysis and building locally):
This is the answer, and it's more serious than noise. The vectorVector I first showed was the v3 baseline; the code that actually runs on AVX-512 hardware (this machine, and the CI perf machines) is the _x86_64_v4 clone. Comparing both clones before/after:
┌────────────┬──────────────────────────────────────────────────────┬───────────────────────────────────────────────────────────────┐
│ │ BEFORE (master) │ AFTER (PR) │
├────────────┼──────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────┤
│ v3 │ branchy scalar (js/jne per elem) │ branchless scalar (cmp/sbb/setl) ✅ │
│ baseline │ │ │
├────────────┼──────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────┤
│ v4 │ true SIMD: vpermt2q split limbs, vpcmpltuq/vpcmpnltq │ loads zmm then vpextrq/vmovq extracts every limb to GPR and │
│ (AVX-512) │ mask compare, 8 elems/iter │ does scalar cmp/sbb/setl per element ❌ │
└────────────┴──────────────────────────────────────────────────────┴───────────────────────────────────────────────────────────────┘
Why is it scalar: std::bit_cast<__int128> + < hands clang a 128-bit scalar compare it can't re-lift into cross-lane AVX-512 mask compares (x86 has no 128-bit-wide integer compare instruction). So on the v4 path the PR de-vectorizes the kernel: master compared 8 Int128s per iteration with vpcmpltuq; the PR extracts each limb back to a GPR and compares scalar. The v3 win and the v4 loss are the same if constexpr acting differently at the two targets.
The LLM initially preferred this other approach just for the 128 case:
#ifdef __SIZEOF_INT128__
if constexpr (Bits == 128 && std::is_same_v<T, integer<Bits, Signed>> && std::endian::native == std::endian::
little)
{
using Hi = std::conditional_t<std::is_same_v<Signed, signed>, int64_t, uint64_t>;
struct Limbs { uint64_t lo; Hi hi; };
auto la = std::bit_cast<Limbs>(lhs);
auto lb = std::bit_cast<Limbs>(rhs);
return (la.hi > lb.hi) | ((la.hi == lb.hi) & (la.lo > lb.lo));
}
#endif
But I prefer generic code as much as possible, because usually it's also simpler and more optimizable (Sorry for the bad variable names, please change them if you use it):
diff --git a/base/base/wide_integer_impl.h b/base/base/wide_integer_impl.h
index 27297dcfb7f..798e1477ad9 100644
--- a/base/base/wide_integer_impl.h
+++ b/base/base/wide_integer_impl.h
@@ -933,19 +933,27 @@ public:
{
if constexpr (should_keep_size<T>())
{
- if (std::numeric_limits<T>::is_signed && (is_negative(lhs) != is_negative(rhs)))
- return is_negative(rhs);
-
integer<Bits, Signed> t = rhs;
- for (unsigned i = 0; i < item_count; ++i)
- {
- base_type rhs_item = get_item(t, big(i));
- if (lhs.items[big(i)] != rhs_item)
- return lhs.items[big(i)] > rhs_item;
+ /// Branchless lexicographic comparison from the most significant limb: no early-out, so the
+ /// columnar comparison kernels stay branchless and vectorize (e.g. to AVX-512 vpcmpq). The top
+ /// limb is compared as signed for signed types, which subsumes the sign check; lower limbs unsigned.
+ base_type l0 = lhs.items[big(0)];
+ base_type r0 = get_item(t, big(0));
+ bool gt;
+ if constexpr (std::is_same_v<Signed, signed>)
+ gt = static_cast<signed_base_type>(l0) > static_cast<signed_base_type>(r0);
+ else
+ gt = l0 > r0;
+ bool eq = l0 == r0;
+ for (unsigned i = 1; i < item_count; ++i)
+ {
+ base_type l = lhs.items[big(i)];
+ base_type r = get_item(t, big(i));
+ gt = gt | (eq & (l > r));
+ eq = eq & (l == r);
}
-
- return false;
+ return gt;
}
else
{
@@ -959,19 +967,25 @@ public:
{
if constexpr (should_keep_size<T>())
{
- if (std::numeric_limits<T>::is_signed && (is_negative(lhs) != is_negative(rhs)))
- return is_negative(lhs);
-
integer<Bits, Signed> t = rhs;
- for (unsigned i = 0; i < item_count; ++i)
- {
- base_type rhs_item = get_item(t, big(i));
- if (lhs.items[big(i)] != rhs_item)
- return lhs.items[big(i)] < rhs_item;
+ /// See the rationale in operator_greater.
+ base_type l0 = lhs.items[big(0)];
+ base_type r0 = get_item(t, big(0));
+ bool lt;
+ if constexpr (std::is_same_v<Signed, signed>)
+ lt = static_cast<signed_base_type>(l0) < static_cast<signed_base_type>(r0);
+ else
+ lt = l0 < r0;
+ bool eq = l0 == r0;
+ for (unsigned i = 1; i < item_count; ++i)
+ {
+ base_type l = lhs.items[big(i)];
+ base_type r = get_item(t, big(i));
+ lt = lt | (eq & (l < r));
+ eq = eq & (l == r);
}
-
- return false;
+ return lt;
}
else
{
Results look good:
Summary
Your approach — delete the #ifdef block, make the generic limb loop branchless — is the right fix. Verified on real object files (same flags, real template instantiations):
┌────────────────────────┬─────────────────────────────────┬────────────────────────────────────────┬──────────────────────────────┐
│ │ v3 baseline │ v4 (AVX-512) │ 256-bit │
├────────────────────────┼─────────────────────────────────┼────────────────────────────────────────┼──────────────────────────────┤
│ master │ branchy scalar (js/jne per │ SIMD vpcmpltuq │ SIMD │
│ │ elem) │ │ │
├────────────────────────┼─────────────────────────────────┼────────────────────────────────────────┼──────────────────────────────┤
│ PR │ branchless scalar ✅ │ scalarized (vpextrq+sbb, ~33% slower) │ unchanged (ifdef only hit │
│ (bit_cast<__int128>) │ │ ❌ │ 128) │
├────────────────────────┼─────────────────────────────────┼────────────────────────────────────────┼──────────────────────────────┤
│ generic branchless │ AVX2-vectorized (vpcmpgtq ymm) │ SIMD vpcmpltuq ✅ │ SIMD + branchless ✅ │
│ loop │ ✅ │ │ │
└────────────────────────┴─────────────────────────────────┴────────────────────────────────────────┴──────────────────────────────┘
PS: 04001_join_reorder_through_expression is fixed. Merging master or just pushing a new commit will fix it.
|
Updating the branch to fix the |
…comparison The bit_cast to __int128 fixed the x86-64-v3 baseline (branchless cmp/sbb instead of a mispredicting per-limb loop) but de-vectorized the x86-64-v4 clones of the columnar comparison kernels: clang cannot re-lift a 128-bit scalar compare into AVX-512 mask compares, so the v4 vectorVector kernel extracted every limb back to GPRs (vpextrq + sbb) instead of comparing 8 values per iteration with vpcmpltuq. This matched the CI perf report: i1 < i2 +15% and i1 >= i2 +17% on x86, while ARM and ORDER BY sped up. Instead, make operator_less and operator_greater branchless for all widths, as suggested in review: compare the most significant limb as signed for signed types (subsuming the sign check), then fold in the lower limbs with carry-free boolean logic and no early-out. This autovectorizes everywhere: the v3 kernels now use AVX2 vpcmpgtq, the v4 kernels keep the same AVX-512 vpcmpltuq/vpcmpgtq/vpcmpeqq shape as master, and the 256-bit kernels become branchless too, instead of being limited to the 128-bit case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wide_integer_from_builtin and wide_integer_from_wide_integer sign-extend
through an `if (rhs < 0)` branch that selects between two separate fill
loops. In columnar conversion loops (ConvertImpl) this data-dependent
branch mispredicts on mixed-sign data and prevents vectorization even
when it predicts well.
Select the extension value (0 or -1) branchlessly instead and use a
single fill loop.
Standalone kernel benchmark of the columnar conversion loop (clang 21,
-march=x86-64-v3, 64k-element blocks, GB/s of processed data):
conversion before after
Int64 -> Int256, random sign 10.3 73.9
Int64 -> Int256, positive only 31.3 73.3
Int128 -> Int256, random sign 12.2 68.0
Int64 -> Int128 61.5 62.2
UInt64 -> UInt128/UInt256 unchanged
The same effect is visible at the query level: with the official
26.6.1 binary, `toInt256(toInt64(cityHash64(number)))` over 100M rows
takes 6.8x the time of the equivalent unsigned conversion in a single
thread, and 3.5x the time of the same conversion with positive-only
values.
Result equivalence with the previous implementation was verified over
36 million checks: all builtin integral types (int8/16/32/64, unsigned
variants) converted to UInt128/Int128/UInt256/Int256 including type
boundary values, 128-to-256-bit widening for all signedness
combinations, narrowing, and constexpr evaluation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The existing checkComparisonAgainstWiderOracle only widens Int128/UInt128 inputs, so every Int256/UInt256 operand had bits 255:128 fixed to sign extension or zero and the upper two limbs of the 4-limb walk in operator_less/operator_greater were never exercised. Add a strictly-ascending oracle (comparisons must agree with the index order, independent of the comparison implementation) and a Comparison256Boundaries test with native Int256/UInt256 pairs whose first differing limb is each of the upper 64-bit chunks: around 1 << 128 and 1 << 192, the unsigned top bit, and signed negatives that share a top limb. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 142/142 (100.00%) · Uncovered code |
There was a problem hiding this comment.
Current perf comparison:
The regression isn't noise, but it's acceptable:
Not noise, and not topK — it's a real, reproducible regression with a clean microarchitectural cause. perf stat nails it:
┌─────────────────┬────────┬────────┬────────────────────────────────┐
│ metric │ master │ pr │ Δ │
├─────────────────┼────────┼────────┼────────────────────────────────┤
│ instructions │ 42.98B │ 42.99B │ ~0 │
├─────────────────┼────────┼────────┼────────────────────────────────┤
│ branches │ 7.46B │ 4.34B │ −42% (branchless, as designed) │
├─────────────────┼────────┼────────┼────────────────────────────────┤
│ branch-misses │ 86.1M │ 86.9M │ ~0 (identical) │
├─────────────────┼────────┼────────┼────────────────────────────────┤
│ L1-dcache-loads │ 10.1B │ 13.3B │ +32% │
└─────────────────┴────────┴────────┴────────────────────────────────┘
The +32% more loads matches the +33% runtime almost exactly. Here's the mechanism:
- The hotspot is miniselect::floyd_rivest_select_loop<…, ColumnVector<UInt128>::greater&> (66–68% self-time) — the ORDER BY … LIMIT 10 selection, and it's comparison-bound in-cache.
- u1's high 64 bits are cityHash64(n,1) → essentially always distinct between rows. So the old early-exit comparator (cmp hi; jne; …) decides on the high limb alone ~100% of the time, never reads the low limb, and — critically — that branch is perfectly predicted (branch-misses are identical between the two builds). It gets a free skip of half the work.
- The branchless combine (seta hi; seta lo; and; or) has no branch to mispredict, but it always reads and compares both limbs — hence +32% L1 loads for zero benefit on this data.
So this is the exact flip side of the signed sort (#7, −55%): there the removed branch was the sign check on random-sign data that mispredicted ~50%, so branchless won big. Here the removed branch was the high-limb-inequality check that predicts ~100% and let you skip the low-limb load, so branchless is pure overhead.
It's the inherent branchless tradeoff, not a bug: you win when the branch is unpredictable (signed sort, columnar SIMD kernels — the PR's main goal) and lose a little when it's perfectly predictable and work-saving (high-cardinality unsigned top-N). Absolute cost is small (~3ms on a 9ms query), and it only hits the scalar comparator path — the columnar WHERE kernels still vectorize. Worth a line in the PR, but the net across the changed paths is still clearly positive.
We can accept a slightly worse single case (when the branch was always predicted correctly and it was a shortcut) but win in all the other ones (unpredictable branches). Also, it only regresses that bit on x86, and ARM sees benefits all around (I'm guessing it's previous code was less simd friendly)
Good job, thanks!
| <max_threads>1</max_threads> | ||
| </settings> | ||
|
|
||
| <query>SELECT count() FROM cmp128 WHERE u1 < u2</query> |
There was a problem hiding this comment.
@raimannma should performance tests for the <= and >= operators be added?
`<=` and `>=` do not share a kernel with `<` and `>`. `operator<=` is `operator_less` combined with `operator_eq`, and `operator>=` is `operator_greater` combined with `operator_eq`, so they exercise code that the existing queries do not cover. The test previously covered `<` on four data shapes, `>` and `>=` on one shape each, and had no coverage of `<=` at all. Add `<=` on the fully random unsigned, random signed, and shared-high-limb shapes, and `>=` on the fully random unsigned shape. Follow-up to a review comment on ClickHouse#109474 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Makes
wide::integercomparisons and sign-extending conversions branchless, so the columnar kernels autovectorize. Combines two changes, as suggested in review (#109475 (review)): the comparison work from this PR and the conversion work merged from #109475.Comparisons (
<,>,<=,>=).wide::integercompares limb-by-limb with an early exit, plus a sign pre-check for signed types. These data-dependent branches make the columnar comparison kernels inFunctionsComparison.hscalar and heavily mispredicting on real data.operator_lessandoperator_greaternow compare the most significant limb as signed for signed types (which subsumes the sign check) and fold in the lower limbs with carry-free boolean logic and no early-out. This autovectorizes for all widths: the x86-64-v3 kernels use AVX2vpcmpgtq, the x86-64-v4 kernels keep the same AVX-512vpcmpltuq/vpcmpgtq/vpcmpeqqshape as before, and the 256-bit kernels become branchless too instead of being limited to the 128-bit case.An earlier revision of this PR used a
std::bit_castto native__int128for the 128-bit case. That fixed the x86-64-v3 baseline but de-vectorized the x86-64-v4 clones of the kernels (clang cannot re-lift a 128-bit scalar compare into AVX-512 mask compares), which showed up in the CI perf report asi1 < i2+15% andi1 >= i2+17% on x86. The generic branchless form replaces it and vectorizes on both v3 and v4.Columnar less-than kernel (clang 21, x86-64-v3): up to ~7x faster on branch-hostile data (
Int128with random signs 8.0 -> 51.8 GB/s,UInt128with 50% shared high limbs 8.4 -> 57.0 GB/s), with no regression on any tested shape, including the old code's best case (predictable early exit) and scalarstd::sortcomparators. Results are bit-for-bit identical to the previous implementation, verified over millions of randomized and boundary pairs for all six comparison operators on all four wide types, plus mixed operand types and constexpr evaluation, on clang 20 and 21.Conversions to wider signed types.
wide_integer_from_builtinandwide_integer_from_wide_integersign-extend through anif (rhs < 0)branch that selects between two separate fill loops. In columnar conversion loops this data-dependent branch mispredicts on mixed-sign data, and the two-loop shape prevents vectorization even when the branch predicts well. The extension value (0 or -1) is now selected branchlessly and a single fill loop is used.Kernel benchmark of the columnar conversion loop (clang 21, x86-64-v3, 64k-element blocks):
Int64toInt256with random signs 10.3 -> 73.9 GB/s (~7x), positive-only 31.3 -> 73.3 GB/s,Int128toInt256with random signs 12.2 -> 68.0 GB/s; unsigned conversions unchanged. Results are bit-for-bit identical, verified over tens of millions of checks covering all builtin integral source types with boundary values, 128-to-256-bit widening for all signedness combinations, narrowing, and constexpr evaluation. The CI perf run on #109475 confirmed the effect at the query level: the newwide_integer_conversiontest is 2-3x faster on both architectures anddecimal_casts #14(toInt256(Decimal)) improved by 17%.Performance tests with branch-hostile data shapes are included for both comparisons and conversions.
Related: #109475
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Improve performance of comparisons (
<,>,<=,>=) of wide integer types (Int128,UInt128,Int256,UInt256and types based on them, such asDecimal128) by up to 7 times, and of converting signed integers toInt256(up to 7 times on mixed-sign data), by making the comparison and sign-extension code branchless.