Skip to content

Reactivate automatic nospecialize for unused arguments #50722

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

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

Conversation

oschulz
Copy link
Contributor

@oschulz oschulz commented Jul 29, 2023

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
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
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 added a commit that referenced this pull request Aug 18, 2023
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
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
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
Member

Then the TODO should be removed, no?

@oschulz
Copy link
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
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
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
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
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants