Handle intrinsics in a more efficent manner. #687
Conversation
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
GuillaumeGomez
approved these changes
May 27, 2025
|
Love the idea! Funnily enough that's what I suggested to @antoyo when opened the issue about the too big |
FractalFir
force-pushed
the
better_intrinsics
branch
from
32f8d9c to
4cb188f
Compare
…ics are now split into separate, architecture-specific functions.
FractalFir
force-pushed
the
better_intrinsics
branch
from
4cb188f to
a579de2
Compare
antoyo
reviewed
May 27, 2025
| @@ -168,25 +168,39 @@ def update_intrinsics(llvm_path, llvmint, llvmint2): | |||
| os.path.dirname(os.path.abspath(__file__)), | |||
| "../src/intrinsic/archs.rs", | |||
| ) | |||
| # A hashmap of all architectures. This allows us to first match on the architecture, and then on the intrisnics. | |||
There was a problem hiding this comment.
Could you please put the intrinsic generation in another commit?
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.
The current implementation of intrinsics is very unoptimized.
In Rust, a match on string gets compiled down to what is effectively an if ladder(maybe we should consider opening an upstream issue about this). This is crazy inefficient, both in terms of the number of basic blocks(and thus compile times), and in the number of comparisons required to match a string(example: matching the 1000 stting will require 1000 comparisons).
The sheer amount of comparisons in
src::intrinsics::llvm::intrinsicstriggers a GCC bug. While trying to recurse on the basic block, GCC overflows its stack.This PR splits that string matching into a couple of functions, dedicated to specific architectures(e.g. ARM) or extensions(e.g. AVX).
This brings both runtime improvements(less comparisons needed) and pretty significant compiletime improvements.
In the master branch, the function in question is the heaviest one in terms of generated LLVM IR, and by a wide margin.
With the patch, the problematic functions are still complex, but are a bit more managable.
On a debug build, this PR reduced build times by ~30 %.
Debug without the patch:
Debug with the patch:
In release, the difference is an over 3x reduction in build times!
Release without patch:
Release with patch:
We still have some tradeoffs to consider. Taking into account the laughably poor codegen for such a match(in both LLVM and GCC), we might just consider using a hash map. There is a very high chance it would have much better runtime & compiletime performance anyway.