-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
codegen: use compiler trampoline closing over method-instance #26565
Conversation
uint8_t compile_traced; // if set will notify callback if this linfo is compiled | ||
jl_fptr_t fptr; // jlcall entry point with api specified by jlcall_api | ||
jl_fptr_t unspecialized_ducttape; // if template can't be compiled due to intrinsics, an un-inferred fptr may get stored here, jlcall_api = JL_API_GENERIC |
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.
Yay :)
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.
Well, technically I just made generated functions more broken but just cared less about it. Although, as long as we make sure there's none included as part of basecompiler, I think it should not have any impact.
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.
So you're saying I have to abandon my plans for the new optimizer to have a dependency on Cxx.jl? Drat!
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.
I was just about to port codegen.cpp to LLVM.jl... /s
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.
Don't let that stop you, @maleadt 😃
a3fc225
to
0ad4b05
Compare
@nanosoldier |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
This should have been a pure merge commit benchmark but the memory regression indicates that #26435 (comment) is active in one of the benchmarks but not the other. I have seen similar things happen multiple times before. I think something is off with nanosoldier... @ararslan |
that's not exactly possible – there's nothing for this commit to merge into, it's already on top of master |
Was that a side mark or did the point of my post not come across (that the baseline benchmark seems to be wrong). |
Nanosoldier lists the specific commits it's using in the report. Looking through Nanosoldier.jl, I don't see any reason to believe that the commits are being misreported, but maybe there's something I'm missing. |
My thinking is that after #26435 (comment) (which the baseline should for sure include), the benchmark |
Maybe actually an issue with that PR? A more recent nanosoldier run on a different PR reports an infinite memory improvement on the same benchmark: #26628 |
I don't see how two PRs against master could result in one giving infinity memory improvement (guaranteeing that it used to allocate before the PR) and one giving infinity memory regression (guaranteeing that it used to not allocate before the PR). |
Replaces jlcall_api with inspection of the invoke closure to see if it is a known entity
Let's just try that again. I've rebased so: @nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
I don't see any commonality between those two reports, so will merge soon. |
Unfortunately, this lovely feature is implicated as the worst offender in the #26767 compilation slow-down. |
Uses a trampoline to reduce the number of jump checks to happen during dynamic dispatch. It used to be done this way initially, but that got lost for awhile after the function-types PR. This brings back the ability to directly call a method without going through the dispatch helper function.
(over)estimated performance jump is pretty good for certain cases: 17.31 ns => 13.82 ns