AK: Always use serenity's math routines, even in lagom builds#26662
Open
nico wants to merge 1 commit intoSerenityOS:masterfrom
Open
AK: Always use serenity's math routines, even in lagom builds#26662nico wants to merge 1 commit intoSerenityOS:masterfrom
nico wants to merge 1 commit intoSerenityOS:masterfrom
Conversation
Some of the functions in AK/Math.h used to implement math routines if AK_OS_SERENITY was defined, but called __builtin_foo() in lagom builds. This was not done consistently. Many functions always implemented math functions without this check, for example: - copysign() - acos(), asin() - cosh(), sinh(), tanh(), acosh, asinh(), atanh() - exp2(), log2(), pow() Move the functions that did that check to not doing it as well. This means these functions now use the same implementation in serenity and lagom. Since lagom is for testing serenity code outside serenity, this seems like a good change. All these functions check for x86 for and use inline assembly there, so this only affects non-x86 lagom builds. It affects these functions: - fmod() - remainder() (now always a TODO(), not just in serenity) - sin(), cos(), tan() - atan(), atan2() - log(), log10() It changes but does not really affect sqrt() -- now a TODO() on non-x86 non-arm non-riscv lagom (i.e. nowhere). As Serenity's implementations of these math functions are currently lower-quality than what's usually in lagom libms, this regresses lagom behavior (performance, precision) on non-x86 for these functions. But only to the level they're at within serenity, and we'll have to improve these functions there anyways.
nico
added a commit
to nico/serenity
that referenced
this pull request
Mar 20, 2026
On my system, with macOS system libm: Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 68ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 56ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 52ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 56ms With serenity libm (SerenityOS#26662): Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 171ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 146ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 153ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 156ms
Contributor
Author
|
This is now down to 16 different LibWeb baselines (from 32 different baselines, and TestComplex failing). Maybe fine to just rebaseline; the diffs look benign (…and we presumably get that output in serenity too). |
Member
This will break with x86's more accurate inline asm impls though, which are used in lagom and don't fail CI. |
spholz
pushed a commit
that referenced
this pull request
Mar 21, 2026
On my system, with macOS system libm: Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 68ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 56ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 52ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 56ms With serenity libm (#26662): Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 171ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 146ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 153ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 156ms
alimpfard
pushed a commit
to alimpfard/serenity
that referenced
this pull request
Mar 24, 2026
On my system, with macOS system libm: Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 68ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 56ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 52ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 56ms With serenity libm (SerenityOS#26662): Running benchmark 'bench_trig_cos'. Completed benchmark 'bench_trig_cos' in 171ms Running benchmark 'bench_trig_cosf'. Completed benchmark 'bench_trig_cosf' in 146ms Running benchmark 'bench_trig_sin'. Completed benchmark 'bench_trig_sin' in 153ms Running benchmark 'bench_trig_sinf'. Completed benchmark 'bench_trig_sinf' in 156ms
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.
Some of the functions in AK/Math.h used to implement math routines if AK_OS_SERENITY was defined, but called __builtin_foo() in lagom builds.
This was not done consistently. Many functions always implemented math functions without this check, for example:
Move the functions that did that check to not doing it as well. This means these functions now use the same implementation in serenity and lagom. Since lagom is for testing serenity code outside serenity, this seems like a good change.
All these functions check for x86 for and use inline assembly there, so this only affects non-x86 lagom builds. It affects these functions:
It changes but does not really affect sqrt() -- now a TODO() on non-x86 non-arm non-riscv lagom (i.e. nowhere).
As Serenity's implementations of these math functions are currently lower-quality than what's usually in lagom libms, this regresses lagom behavior (performance, precision) on non-x86 for these functions. But only to the level they're at within serenity, and we'll have to improve these functions there anyways.
Assuming CI likes this, it'll make it easier to work on #25934 in lagom builds. And it reduces config matrix size for the math routines.