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

macrocall: Specialize macro invocations like normal function calls #57782

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

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Mar 15, 2025

Inspired by #57781 (kudos to @nsajko for another good find)

jl_invoke behaves like Expr(:invoke, ...) not Core.invoke, so this was bypassing the default specialization for a macro and only ever invoked de-specialized MethodInstances.

Change this to jl_apply so that we specialize as normal, which should dramatically improve inference and not lead to much extra code since our AST is very homogeneous (Expr + typeof.(literals), IIUC).

`jl_invoke` behaves like `Expr(:invoke, ...)` not `Core.invoke`, so we
were (accidentally?) bypassing the default specialization for a `macro`
and only ever invoked de-specialized MethodInstances.

Change this to `jl_apply` so that we specialize as normal, which should
dramatically improve inference and not lead to much extra code since our
AST is very homogeneous (`Expr` + typeof.(literals), IIUC)
@topolarity topolarity force-pushed the ct/specialize-macrocall branch from dfdd613 to 75c3101 Compare March 15, 2025 13:31
@topolarity topolarity added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 invalidations backport 1.12 Change should be backported to release-1.12 labels Mar 15, 2025
@vtjnash
Copy link
Member

vtjnash commented Mar 15, 2025

only ever invoked de-specialized MethodInstances

That was sort of the point though, since inference is supposed to reject these also anyways, so attempted specialization here is expected to be just wasted effort

@topolarity
Copy link
Member Author

inference is supposed to reject these also anyways, so attempted specialization here is expected to be just wasted effort

Why the invalidations in #57781 then?

@nsajko
Copy link
Contributor

nsajko commented Mar 19, 2025

This PR doesn't seem to prevent any invalidation - the invalidations are exactly the same before and after applying this change on top of current master, 6eef79c.

@nsajko
Copy link
Contributor

nsajko commented Mar 19, 2025

Furthermore, I can confirm the invalidation of macro MethodInstances, such as "MethodInstance for var\"@ip_str\"(::LineNumberNode, ::Module, ::Any)", is still present on master (and with this PR).

@JeffBezanson
Copy link
Member

only ever invoked de-specialized MethodInstances

I'm not sure why the code is written this way (seems to be from c79e34c) but I don't think this is true? This code path does end up doing specialization. But then there is a check for functions starting with '@', and we skip inference in that case. But then I'm not sure where the backedges come from. There must be another code path that ends up invoking inference. But anyway, you could try disabling the '@' check in gf.c.

@JeffBezanson
Copy link
Member

Oh we also insert nospecialize on macro arguments in lowering.

@topolarity
Copy link
Member Author

Oh we also insert nospecialize on macro arguments in lowering.

Ah yep - Looks like that was auto-qualifying any macro for the "single specialization" pre-compile hint: #57833

@KristofferC KristofferC mentioned this pull request Mar 20, 2025
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 invalidations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants