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

internal: more uniform usage of jlcall ABI again #31984

Merged
merged 1 commit into from
May 17, 2019
Merged

internal: more uniform usage of jlcall ABI again #31984

merged 1 commit into from
May 17, 2019

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 9, 2019

A bit of history: Before the addition of function types, a call to a function would always use the API (jl_value_t *f, jl_value_t *argv[], int nargv), regardless of whether it was a generic call or a closure or an invoke or interpreted. Over time since then, that become one of several calling conventions, each just a minor variation away: (jl_value_t *argv[], int nargv), (void *context, jl_value_t *argv[], int nargv), (jl_value_t *f, jl_value_t *argv[], int nargs), (jl_value_t **context, jl_value_t *f, jl_value_t *argv[], int nargv), and so on. That was fairly nice to getting stuff ported over, since it provided a simpler transition, but led to needing an increasing number of transformational methods to just shift arguments around (such as jl_apply_2va, which gets removed here). And that was starting to feel tricky to me to manage the possible N-to-N conversions that may be needed between each of them, and to optimize all of those.

This PR aims to cut back down again on the amount of variety in our primary (non internal/specsig) ABI by moving the varying 'context' to the end and usually keeping the first (function) argument separate. This gives the basic call signature form: (jl_value_t *func, jl_value_t *argv[], int nargv, void *optional_context) (where the last parameter can range over a few possible types depending on the target, or may be undef and dropped when it is known that the callee will ignore the value). This lets us switch callee target type (e.g. for incremental compilation and linking) simply by adding / dropping / replacing / ignoring that trailing parameter, and not needing to potentially spill everything to the stack (e.g. letting us remove jl_apply_2va).

This doesn't alter jl_apply() / jl_call / etc., so external consumers should be generally unaffected.

@vtjnash vtjnash requested a review from JeffBezanson May 9, 2019 22:17
src/builtins.c Outdated Show resolved Hide resolved
src/codegen_shared.h Outdated Show resolved Hide resolved
JL_GC_POP();
return ret;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yeah, this was a super ugly function.

@vtjnash
Copy link
Member Author

vtjnash commented May 14, 2019

Just to check: @nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@vtjnash
Copy link
Member Author

vtjnash commented May 15, 2019

Curious, it doesn't seem that we have any benchmarks affected by the simplified splatting calling. But this seems to have broken something about parse_json and binary_tree. I'll have to look into those.

@vtjnash
Copy link
Member Author

vtjnash commented May 15, 2019

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@JeffBezanson
Copy link
Member

Looks good now. Did parse_json and binary_tree actually reveal something, or were they noise?

@vtjnash
Copy link
Member Author

vtjnash commented May 16, 2019

yeah, I had accidentally disabled an optimization. it's all good now, so I'll plan to merge tomorrow

src/gf.c Outdated Show resolved Hide resolved
src/codegen_shared.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants