-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
implement sret_union ABI for pointer-ful types
#55045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
topolarity
wants to merge
11
commits into
master
Choose a base branch
from
ct/union-sret-abi
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+863
−557
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
3ad2529 to
e33c792
Compare
vtjnash
added a commit
that referenced
this pull request
Sep 4, 2024
20bc368 to
1f8932f
Compare
0bd445a to
9c008c1
Compare
3153817 to
f6762fa
Compare
5530839 to
eec16be
Compare
|
|
||
| // bitcast whatever Ptr kind x might be (even if it is part of a union) into Ptr{Cvoid} | ||
| // given that the caller already had emit_cpointercheck on this branch, so that | ||
| // the conversion is guaranteed to be valid on this runtiem branch |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
| // the conversion is guaranteed to be valid on this runtiem branch | |
| // the conversion is guaranteed to be valid on this runtime branch |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
sret_union ABI for pointer-ful typessret_union ABI for pointer-ful types
This is a combination the existing: - `sret` ABI (which can stack-allocate a _single_ pointerful type) - `union` ABI (which can stack-allocate many _pointer-free_ types) This provides some nice speed-ups for temporary "wrappers": ```julia const v = Any[] @noinline maybe_wrapped(i) = (i % 32 != 0) ? Some(v) : nothing function foo() count = 0 for i = 1:1_000_000 count += (maybe_wrapped(i) !== nothing) ? 1 : 0 end return count end ``` On this PR this gives: ```julia julia> @Btime foo() 1.675 ms (0 allocations: 0 bytes) 968750 ``` compared to current master: ```julia julia> @Btime foo() 6.877 ms (968750 allocations: 14.78 MiB) 968750 ``` The most outstanding TODO here is what to do about PHI nodes. Right now, if the incoming `Union{...}` type has a pointer-containing type then the object is forced to be boxed, even if the object at run-time is actually pointer-free. But that's just a band-aid - it means we introduce new boxes where we didn't have them before, which is a regression that almost certainly to be fixed before landing this. Co-authored-by: Gabriel Baraldi <[email protected]>
always need to call typeassert / update type
A concrete-typed `cgval_t` can only be statically boxed or unboxed, while a `union-split` value can dynamically be either.
This variable was accidentally dropped in the change to use the "julia.return_roots" attribute.
… where the next crash seems likely to be caused by
eec16be to
dd88ee8
Compare
Member
|
Hopefully getting closer now. Still reaching a segfault trying to observe the parent field in this offsetvector after the phic reload: julia> isdefined(Main, :OffsetArrays) || @eval Main include("testhelpers/OffsetArrays.jl"); using .Main.OffsetArrays
julia> testf() = for a in @noinline (["foo", "Bar"], OffsetVector(["foo", "Bar"], 0:1))
try; error(); catch; end
@noinline a == a
end
testf (generic function with 1 method)
julia> testf()
[3998236] signal 11 (1): Segmentation fault
in expression starting at REPL[6]:1
size at ./essentials.jl:10 [inlined]
axes at ./abstractarray.jl:102 [inlined]
axes at /home/vtjnash/julia/test/testhelpers/OffsetArrays.jl:504 [inlined]
== at ./abstractarray.jl:3049
testf at ./REPL[5]:3 |
d271970 to
365c3f9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
compiler:codegen
Generation of LLVM IR and native code
needs pkgeval
Tests for all registered packages should be run with this change
performance
Must go faster
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.
This effectively expands our existing
unionABI to cover both of these existing cases:sretABI (which can stack-allocate a single pointer-ful type)unionABI (which can stack-allocate many pointer-free types)This provides some nice speed-ups for temporary "wrappers":
On this PR this gives:
compared to current master:
TODO:
The most outstanding TODO here is what to do about ϕ-nodes. Right now, if the incoming
Union{...}type has a pointer-containing type then this change forces the incoming object to be boxed, even if the object at run-time is actually pointer-free.But that's just a band-aid so the code works - it introduces new boxes where we didn't have them before, which is a regression that almost certainly needs to be fixed before landing this.