Skip to content
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

make memorynew intrinsic #56803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oscardssmith
Copy link
Member

Attempt to split up #55913 into 2 pieces. This piece now only adds the memorynew intrinsic without any of the optimizations enabled by #55913. As such, this PR should be ready to merge now. (and will make #55913 smaller and simpler)

@oscardssmith oscardssmith added the arrays [a, r, r, a, y, s] label Dec 11, 2024
@KristofferC KristofferC added the needs pkgeval Tests for all registered packages should be run with this change label Dec 11, 2024
src/common_symbols2.inc Outdated Show resolved Hide resolved
src/pipeline.cpp Outdated Show resolved Hide resolved
src/rtutils.c Show resolved Hide resolved
test/core.jl Show resolved Hide resolved
base/genericmemory.jl Outdated Show resolved Hide resolved
base/essentials.jl Outdated Show resolved Hide resolved
doc/src/manual/performance-tips.md Show resolved Hide resolved
doc/src/manual/performance-tips.md Show resolved Hide resolved
@oscardssmith oscardssmith force-pushed the os-memorynew-light branch 2 times, most recently from 69d40b6 to 4ec80ce Compare December 11, 2024 18:00
@oscardssmith oscardssmith changed the title make memorynew intrinsic make memorynew intrinsic (part 1) Dec 11, 2024
@gbaraldi
Copy link
Member

This is much worse than the current implementation btw. For this you at least need a specialized builtin implementation in codegen, even if it just forwards the arguments.

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Jeff Bezanson <[email protected]>
Co-authored-by: Gabriel Baraldi  <[email protected]>
@oscardssmith
Copy link
Member Author

To prevent this PR from being a regression (and to fix the LLVM names test, I think the right way to go is to add the dynamic length version of codegen to this PR. It always goes through C (so LLVM won't be able to delete the whole allocation), but this way this PR on it's own is ~3ns faster to allocate arrays than master without the glory/risk of LLVM deleting the allocation entirely.

@oscardssmith
Copy link
Member Author

oscardssmith commented Dec 12, 2024

@KristofferC I don't think this PR needs a pkgeval. It doesn't have any of the risks of #55913 wrt weird miscompiles. (that said, if you disagree, feel free to run it).

@oscardssmith oscardssmith added performance Must go faster compiler:codegen Generation of LLVM IR and native code labels Dec 12, 2024
@oscardssmith oscardssmith changed the title make memorynew intrinsic (part 1) make memorynew intrinsic Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants