BUG: Canonicalize long-double padding across storage paths - #116
Conversation
ngoldbaum
left a comment
There was a problem hiding this comment.
This is mergeable as-is. I left some comments that can be addressed here before merging or in followups as you see fit. The aligned speedup is almost definitely a followup candidate.
| quad_value_set_longdouble(quad_value *value, long double input) | ||
| { | ||
| quad_value_zero(value); | ||
| value->longdouble_value = input; |
There was a problem hiding this comment.
should this perhaps call quad_longdouble_store instead of this direct store? Since I think in principle the same issue could happen here with the padding bytes never getting written.
| #include <float.h> | ||
| #include <stddef.h> | ||
| #include <string.h> | ||
|
|
||
| #ifdef _MSC_VER | ||
| #include <intrin.h> | ||
| #endif |
There was a problem hiding this comment.
Should these get included outside the extern "C" block? My AI model tells me that is more robust to possible issues in C++ standard libraries.
| #if (defined(__i386__) || defined(__x86_64__)) && LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384 | ||
| // x87 extended precision occupies bytes 0..9; the remaining bytes are padding. | ||
| memset(dst, 0, sizeof(value)); | ||
| memcpy(dst, &value, 10); |
There was a problem hiding this comment.
This 10 should probably be a macro with a name like NUM_DIGITS_X87 or something like that
| } | ||
|
|
||
| static inline void | ||
| quad_longdouble_store(void *dst, long double value) |
There was a problem hiding this comment.
My AI model suggests specializing this into aligned and unaligned variants, with a store-then-memset variant for x87:
static inline void
quad_longdouble_store_aligned(void *dst, long double value)
{
#if (defined(__i386__) || defined(__x86_64__)) && LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384
// Needs no barrier: the trailing memset is the last store to the padding
// bytes, so it cannot be eliminated as dead, and overlapping stores are
// never reordered.
*(long double *)dst = value;
memset((char *)dst + 10, 0, sizeof(value) - 10);
#else
quad_longdouble_store(dst, value);
#endif
}
static inline void
quad_value_store(void *dst, const quad_value *value, QuadBackendType backend)
{
if (backend == BACKEND_SLEEF) {
memcpy(dst, &value->sleef_value, sizeof(value->sleef_value));
}
else {
quad_longdouble_store(dst, value->longdouble_value);
}
}
static inline void
quad_value_store_aligned(void *dst, const quad_value *value, QuadBackendType backend)
{
if (backend == BACKEND_SLEEF) {
memcpy(dst, &value->sleef_value, sizeof(value->sleef_value));
}
else {
quad_longdouble_store_aligned(dst, value->longdouble_value);
}
}This is apprently substantially faster for the common aligned case. You can also then use quad_longdouble_store_aligned in a number of spots throughout the codebase where you know by construction that you have aligned buffers.
There was a problem hiding this comment.
Good catch, I added this and it gave pretty decent gains on some ops
|
Cool, all green, merging this in! |
Closes #111
The shared helpers in
quad_common.hnow handle:quad_valueinitializationThe x86/x86-64 x87 hot path uses the known representation:
This initializes the entire slot and then copies the 10 meaningful x87 bytes.
Other extended-precision ABIs use a conservative representation-independent fallback. Long-double formats without padding retain a direct copy. SLEEF storage remains unchanged.
Performance
SLEEF is unaffected because it continues to use its existing 16-byte copy.
On this x86-64 host, the optimized x87 canonical store added approximately 3–8% to typical long-double arithmetic kernels. Output-heavy operations show a larger relative cost (
modfabout 40%, float64-to-Quad casting about 70%) because they perform very little computation beyond writing one or two results.The canonicalization is limited to padded x87 long-double output storage; non-padded long-double formats use the direct-copy path.