make `memorynew` intrinsic #56803

oscardssmith · 2024-12-11T15:12:51Z

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)

src/common_symbols2.inc

src/pipeline.cpp

src/rtutils.c

test/core.jl

base/genericmemory.jl

base/essentials.jl

doc/src/manual/performance-tips.md

gbaraldi · 2024-12-12T02:43:02Z

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 · 2024-12-12T03:21:46Z

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 · 2024-12-12T03:22:50Z

@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).

KristofferC · 2024-12-12T10:17:04Z

I am surprised you would say that and I strongly disagree. The other PR was also just about to be merged before PkgEval showed it was quite buggy so it would be unfortunate to repeat the same mistake again. The whole emit_memorynew looks like it could cause miscompilation issues?

KristofferC · 2024-12-12T10:18:34Z

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.

Has this been done now? It is hard to follow the evolution of the PR when the original commit gets amended over and over.

KristofferC · 2024-12-12T10:25:15Z

Also, benchmarks should be run here since it touches some core array functionality and to show the performance benefit and that no regressions gets introduced (e.g. from the seemingly new boundscheck).

Compiler/src/tfuncs.jl

src/rtutils.c

src/codegen.cpp

oscardssmith · 2024-12-12T13:22:25Z

The whole emit_memorynew looks like it could cause miscompilation issues?

@KristofferC the reason I say this version is much lower risk is that none of the segfaults that we were seeing come from the emit_memorynew function. (i.e. if you deleted the LLVM optimization passes we do, it would compile correctly). The cause of potential miscompiles in the other PR comes from the fact that if we teach the compiler how to do a Memory allocation completely (i.e. without the ccall that exists in this version), the compiler is able to analyze it and remove the allocation if it believes the allocation doesn't leak. This version of the PR keeps it as an "opaque" object allocation so the class of miscompile we were seeing can't occur. (and hence I think the regular Julia tests are likely to be sufficient for testing the PR).

Has this been done now

yes.

topolarity · 2024-12-12T13:57:46Z

src/cgutils.cpp

+        Value *add_with_overflow = ctx.builder.CreateCall(intr, {nel_unboxed, nbytes});
+        nbytes = ctx.builder.CreateExtractValue(add_with_overflow, 0);
+        Value *overflow1 = ctx.builder.CreateExtractValue(add_with_overflow, 1);
+        overflow = ctx.builder.CreateOr(overflow, overflow1);


I know the issues we ran into on the last PR were mostly from complicated interaction with a downstream pass, but I think it is worth writing unit tests that target this specific path in codegen

Can you add those please?

(i.e. test nel == 0 vs. nel != 0, isunion vs. !isunion, etc.)

sure (although just being able to build Julia is a pretty good test that none of these cases are broken)

I've actually realized that this is fairly difficult to test for directly in ways that the compiler won't cheat at. i.e. inference knows that memorynew(Memory{Int}, 5) returns a Memory{Int}, so testing return type will just be optimized out unless I go through a bunch of effort to try to fool the compiler (which also definitely would have segfaulted if it couldn't allocate a Memory).

topolarity · 2024-12-12T14:02:54Z

src/codegen.cpp

+        if (inst == NULL)
+            return false;


When does this happen?

this means the type isn't Const (i.e. for code like
Memory{rand((Int, Float64))}(undef, 1)

Isn't that the check 3 lines above?

oh, right. this case might be for Memory{1}(undef, 1) or something like that where it is Const, but not valid.

oscardssmith · 2024-12-12T18:25:42Z

@nanosoldier runtests()

oscardssmith · 2024-12-12T21:46:35Z

turns out that we want #56817 before this PR anyway since the segfaults on the OG memorynew PR were just a bug that cleaning up the IR magically is enough to trigger.

vchuravy · 2024-12-12T22:50:05Z

and hence I think the regular Julia tests are likely to be sufficient for testing the PR

You are still adding more precise side effects in the modelling of the intrinsic that could have knock on effects down the road. I agree that this PR is lower risk, but PkgEval is not that expensive and it really helps being able to have that strong signal.

maleadt · 2024-12-13T09:13:00Z

turns out that we want #56817 before this PR anyway since the segfaults on the OG memorynew PR were just a bug that cleaning up the IR magically is enough to trigger.

Better wait before triggering PkgEval then? I had to restart the server, so your submission was lost; you'll need to re-trigger (either right away, or after #56817 is merged).

oscardssmith · 2024-12-13T14:47:23Z

@nanosoldier runtests()

oscardssmith · 2024-12-14T05:29:57Z

no clue what's up with packageeval. if it doesn't come back within the next day, I'm merging without it. I have at least 1 PR (and hopefully another few after) that depend on this that I'm hoping to land in the 1.12 window.

oscardssmith added the arrays [a, r, r, a, y, s] label Dec 11, 2024

KristofferC added the needs pkgeval Tests for all registered packages should be run with this change label Dec 11, 2024

KristofferC reviewed Dec 11, 2024

View reviewed changes

oscardssmith force-pushed the os-memorynew-light branch 2 times, most recently from 69d40b6 to 4ec80ce Compare December 11, 2024 18:00

oscardssmith changed the title ~~make memorynew intrinsic~~ make memorynew intrinsic (part 1) Dec 11, 2024

make memorynew intrinsic

aeaf45e

Co-authored-by: Jameson Nash <[email protected]> Co-authored-by: Jeff Bezanson <[email protected]> Co-authored-by: Gabriel Baraldi <[email protected]>

oscardssmith force-pushed the os-memorynew-light branch from 4ec80ce to aeaf45e Compare December 12, 2024 03:18

oscardssmith added performance Must go faster compiler:codegen Generation of LLVM IR and native code labels Dec 12, 2024

oscardssmith changed the title ~~make memorynew intrinsic (part 1)~~ make memorynew intrinsic Dec 12, 2024

vchuravy reviewed Dec 12, 2024

View reviewed changes

Compiler/src/tfuncs.jl Outdated Show resolved Hide resolved

src/rtutils.c Show resolved Hide resolved

src/codegen.cpp Outdated Show resolved Hide resolved

address review

4a2f5f3

topolarity reviewed Dec 12, 2024

View reviewed changes

Merge branch 'master' into os-memorynew-light

d77bb2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `memorynew` intrinsic #56803

make `memorynew` intrinsic #56803

oscardssmith commented Dec 11, 2024

gbaraldi commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

oscardssmith commented Dec 12, 2024 •

edited

Loading

KristofferC commented Dec 12, 2024 •

edited

Loading

KristofferC commented Dec 12, 2024

KristofferC commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

topolarity Dec 12, 2024 •

edited

Loading

oscardssmith Dec 12, 2024

oscardssmith Dec 14, 2024

topolarity Dec 12, 2024

oscardssmith Dec 12, 2024

topolarity Dec 12, 2024

oscardssmith Dec 12, 2024

oscardssmith commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

vchuravy commented Dec 12, 2024

maleadt commented Dec 13, 2024

oscardssmith commented Dec 13, 2024

oscardssmith commented Dec 14, 2024

make memorynew intrinsic #56803

Are you sure you want to change the base?

make memorynew intrinsic #56803

Conversation

oscardssmith commented Dec 11, 2024

gbaraldi commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

oscardssmith commented Dec 12, 2024 • edited Loading

KristofferC commented Dec 12, 2024 • edited Loading

KristofferC commented Dec 12, 2024

KristofferC commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

topolarity Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

oscardssmith Dec 12, 2024

Choose a reason for hiding this comment

oscardssmith Dec 14, 2024

Choose a reason for hiding this comment

topolarity Dec 12, 2024

Choose a reason for hiding this comment

oscardssmith Dec 12, 2024

Choose a reason for hiding this comment

topolarity Dec 12, 2024

Choose a reason for hiding this comment

oscardssmith Dec 12, 2024

Choose a reason for hiding this comment

oscardssmith commented Dec 12, 2024

oscardssmith commented Dec 12, 2024

vchuravy commented Dec 12, 2024

maleadt commented Dec 13, 2024

oscardssmith commented Dec 13, 2024

oscardssmith commented Dec 14, 2024

make `memorynew` intrinsic #56803

make `memorynew` intrinsic #56803

oscardssmith commented Dec 12, 2024 •

edited

Loading

KristofferC commented Dec 12, 2024 •

edited

Loading

topolarity Dec 12, 2024 •

edited

Loading