Skip to content

Reactivate automatic nospecialize for unused arguments#50722

Open
oschulz wants to merge 1 commit intoJuliaLang:masterfrom
oschulz:nospecialize-unused-args
Open

Reactivate automatic nospecialize for unused arguments#50722
oschulz wants to merge 1 commit intoJuliaLang:masterfrom
oschulz:nospecialize-unused-args

Conversation

@oschulz
Copy link
Copy Markdown
Contributor

@oschulz oschulz commented Jul 29, 2023 •
edited
Loading

Reactivate automatic nospecialize for unused arguments, to avoid unnecessary code generation in use cases like

foo(A::Array, B::Array) = length(A) * length(B)
foo(A::Array, ::Number) = length(A)

(let's see if the bug mentioned in the code is gone by now).

Maybe even use the new @nospecializeinfer? (I don't know how to do this in C though :-) ).

CC @JeffBezanson , @LilithHafner (our discussion at JuliaCon).

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Jul 31, 2023

clearly the bug is still present:

Error in testset ambiguous:
Test Failed at /cache/build/default-amdci5-0/julialang/julia-master/julia-908016c492/share/julia/test/ambiguous.jl:289
  Expression: f(0)
    Expected: MethodError
  No exception thrown
Error in testset ambiguous:
Test Failed at /cache/build/default-amdci5-0/julialang/julia-master/julia-908016c492/share/julia/test/ambiguous.jl:290
  Expression: f(pi)
    Expected: MethodError
  No exception thrown
Error in testset ambiguous:
Test Failed at /cache/build/default-amdci5-0/julialang/julia-master/julia-908016c492/share/julia/test/ambiguous.jl:289
  Expression: f(0)
    Expected: MethodError
  No exception thrown
Error in testset ambiguous:
Test Failed at /cache/build/default-amdci5-0/julialang/julia-master/julia-908016c492/share/julia/test/ambiguous.jl:290
  Expression: f(pi)
    Expected: MethodError
  No exception thrown
Error in testset precompile:
Test Failed at /cache/build/default-amdci5-0/julialang/julia-master/julia-908016c492/share/julia/test/precompile.jl:1628
  Expression: precompile(Tuple{typeof(f46778), Int, DataType})

@oschulz
Copy link
Copy Markdown
Contributor Author

oschulz commented Jul 31, 2023

Darn! I'm afraid fixing that is beyond my competence ...

@brenhinkeller brenhinkeller added the performance Must go faster label Aug 6, 2023
@vtjnash vtjnash mentioned this pull request Aug 17, 2023
Merged
vtjnash added a commit that referenced this pull request Aug 18, 2023
@vtjnash
fix missing methods in ml_matches results (#50962)
762801c 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
@vchuravy vchuravy reopened this Aug 18, 2023
@vchuravy vchuravy force-pushed the nospecialize-unused-args branch from 908016c to 19090e7 Compare August 18, 2023 17:35
KristofferC pushed a commit that referenced this pull request Mar 26, 2025
@vtjnash @KristofferC
fix missing methods in ml_matches results (#50962)
3b03ade 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
(cherry picked from commit 762801c)
@KristofferC KristofferC force-pushed the nospecialize-unused-args branch from 19090e7 to 1fae15a Compare March 26, 2025 20:26
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Mar 26, 2025

I was pretty sure we'd already concluded this was actually pretty bad for performance, so we'd closed it already

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Mar 26, 2025

Then the TODO should be removed, no?

@oschulz
Copy link
Copy Markdown
Contributor Author

oschulz commented Mar 27, 2025

I was pretty sure we'd already concluded this was actually pretty bad for performance, so we'd closed it already

If this is bad for performance, should things like @nospecialize also be avoided on unused arguments, in general? Just curious.

nickrobinson251 pushed a commit to RelationalAI/julia that referenced this pull request Mar 27, 2025
@vtjnash @nickrobinson251
fix missing methods in ml_matches results (JuliaLang#50962)
f4e738a 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
JuliaLang#50722 (comment)
(cherry picked from commit 762801c)
nickrobinson251 added a commit to RelationalAI/julia that referenced this pull request Mar 27, 2025
@nickrobinson251 @vtjnash
fix missing methods in ml_matches results (JuliaLang#50962) (#227)
36d926f 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
JuliaLang#50722 (comment)
(cherry picked from commit 762801c)

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
@vtjnash @KristofferC
fix missing methods in ml_matches results (#50962)
abbb6cf 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
(cherry picked from commit 762801c)
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
@vtjnash @KristofferC
fix missing methods in ml_matches results (#50962)
6908714 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
(cherry picked from commit 762801c)
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
@vtjnash @KristofferC
fix missing methods in ml_matches results (#50962)
9a73b3a 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
(cherry picked from commit 762801c)
KristofferC pushed a commit that referenced this pull request Jul 3, 2025
@vtjnash
fix missing methods in ml_matches results (#50962)
d4a48f0 
This was resulting in it being too aggressive at filtering out
"duplicate" results, resulting in possible inference mistakes or missing
guardsig entries.

Fixes:
#50722 (comment)
(cherry picked from commit 762801c)
@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Feb 21, 2026

This PR has had no activity for over six months.

@oschulz Are you still interested in working on this PR?

If you're no longer interested in working on this PR, no worries! Just let me know, and I can close it.

@oschulz
Copy link
Copy Markdown
Contributor Author

oschulz commented Feb 22, 2026

@oschulz Are you still interested in working on this PR?

I'm interested in this PR, but I don't think I know the compiler at the level to make it work. I started with with help from @vtjnash at a JuliaCon Hackathon and I think this needs an expert eye.

@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Feb 22, 2026

@vtjnash @vchuravy What do you think - is this still desired?

(If not, then I agree with Kristoffer that we might as well remove the commented-out TODO.)

@oschulz
Copy link
Copy Markdown
Contributor Author

oschulz commented Feb 22, 2026

In principle there's no reason to specialize if an argument value is not used at all (not even via a type parameter) since the argument then only acts as a "method selector" (a common pattern, I'd say), correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performance Must go faster status: waiting for PR author

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@oschulz @vtjnash @KristofferC @DilumAluthge @vchuravy @brenhinkeller