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

Fix tfunc tables for fptoui and fptosi #30787

Merged
merged 1 commit into from
Jan 21, 2019
Merged

Fix tfunc tables for fptoui and fptosi #30787

merged 1 commit into from
Jan 21, 2019

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 21, 2019

While working on an interpreter (JuliaDebug/ASTInterpreter2.jl#37) I noticed that fptoui and fptosi only support the 2-argument form:

julia> f(x) = Base.fptoui(x)
f (generic function with 1 method)

julia> f(3.0)
ERROR: error compiling f: intrinsic #55 fptoui: wrong number of arguments
Stacktrace:
 [1] top-level scope at REPL[2]:1

However, the 1-argument form is still present in inference's tfunc tables. I'm going to mark this for backporting since that behavior seems present even on 0.7. But this is very low priority, I am not aware of anything other than JuliaDebug/ASTInterpreter2.jl#37 that needs this (and there I have a workaround).

Only the 2-argument form is supported by intrinsics.cpp, so this deletes
the 1-argument forms from Core.Compiler's tfunc tables.
@vtjnash
Copy link
Member

vtjnash commented Jan 21, 2019

Yep, 2-arg should be deprecated, so this is just a theoretical memory improvement (it'll delete more dead code after the error was thrown), with no actual impact.

@timholy
Copy link
Member Author

timholy commented Jan 21, 2019

2-arg should be deprecated

You mean 1-arg, right?

@timholy timholy merged commit 289ef1d into master Jan 21, 2019
@timholy timholy deleted the teh/fix_tfunc branch January 21, 2019 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants