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

Odd difference in ccall'ing function from a parameterized method versus one with hardcoded types #29400

Closed
pkofod opened this issue Sep 27, 2018 · 5 comments
Labels
compiler:codegen Generation of LLVM IR and native code error handling Handling of exceptions by Julia or the user

Comments

@pkofod
Copy link
Contributor

pkofod commented Sep 27, 2018

Sorry for the slightly weird title, but I have no idea what the problem is, so maybe we can change the title when someone more clever than I am stops by :)

julia> __ldexp_exp(x::Float64, i::T) where T = ccall(("__ldexp_exp", Base.Math.libm), Float64, (Float64, T), x, i)
__ldexp_exp (generic function with 1 method)

julia> __ldexp_exp(0.245,-1)
2.4519928653854222e55

julia> Base.Math._ldexp_exp(0.245,-1)
2.4519928653854222e55

julia> ccall(("__ldexp_exp", Base.Math.libm), Float64, (Float64, typeof(-1)), 0.245, -1)
1.9872231581449074e233

See, I'm trying to create a Julia port of what is called here

_ldexp_exp(x::Float64, i::T) where T = ccall(("__ldexp_exp", libm), Float64, (Float64, T), x, i)
but this weird problem showed up. _ldexp_exp is defined here https://github.com/JuliaMath/openlibm/blob/cca41bc1abd01804afa4862bbd2c79cc9803171a/src/k_exp.c#L75

The following thing might help people who know about this stuff decipher what's going on

julia> __ldexp_exp_T(x::Float64, i::T) where T = ccall(("__ldexp_exp", Base.Math.libm), Float64, (Float64, T), x, i)
__ldexp_exp_T (generic function with 1 method)

julia> __ldexp_exp(x::Float64, i::T) where T = ccall(("__ldexp_exp", Base.Math.libm), Float64, (Float64, typeof(-1)), x, i)
__ldexp_exp (generic function with 1 method)

julia> @code_llvm __ldexp_exp(0.245, -1)

; Function __ldexp_exp
; Location: REPL[31]:1
define double @julia___ldexp_exp_392966994(double, i64) {
top:
 %2 = call double inttoptr (i64 5074017824 to double (double, i64)*)(double %0, i64 %1)
 ret double %2
}

julia> @code_llvm __ldexp_exp_T(0.245, -1)

; Function __ldexp_exp_T
; Location: REPL[30]:1
define double @julia___ldexp_exp_T_392966995(double, i64) {
top:
 %gcframe = alloca %jl_value_t addrspace(10)*, i32 3
 %2 = bitcast %jl_value_t addrspace(10)** %gcframe to i8*
 call void @llvm.memset.p0i8.i32(i8* %2, i8 0, i32 24, i32 0, i1 false)
 %3 = call %jl_value_t*** inttoptr (i64 4546681056 to %jl_value_t*** ()*)() #3
 %4 = getelementptr %jl_value_t addrspace(10)*, %jl_value_t addrspace(10)** %gcframe, i32 0
 %5 = bitcast %jl_value_t addrspace(10)** %4 to i64*
 store i64 2, i64* %5
 %6 = getelementptr %jl_value_t**, %jl_value_t*** %3, i32 0
...
}

Hardcoding Int64 produces the same result as typeof(-1).

If I'm somehow breaking the rules of ccall I just don't know why. If it's a bug, I don't know what's happening either, so... 🤷‍♂️

@yuyichao
Copy link
Contributor

I'm not sure if this the parametric type one is intented to be supported since C function only have one type that does not depend on the caller. The T is interpreted as a pointer in this case though, which should probably be at least an error if this is not supported now.

For this particular case, all of the code are wrong, it should use Int32 instead, even though the result you get from Int64 is also correct despite the wrong signature.

@pkofod
Copy link
Contributor Author

pkofod commented Sep 27, 2018

I'm not sure if this the parametric type one is intented to be supported since C function only have one type that does not depend on the caller. The T is interpreted as a pointer in this case though, which should probably be at least an error if this is not supported now.

For this particular case, all of the code are wrong, it should use Int32 instead, even though the result you get from Int64 is also correct despite the wrong signature.

You're right, the T is odd and wrong: it should be Int32 to match the int in the C code. Thanks for the reply.

I think this

The T is interpreted as a pointer in this case though, which should probably be at least an error if this is not supported now.

means that the behavior is not intended, there should have been thrown an error somewhere, but I must admit I don't fully understand why T is interpreted as a pointer here.

@yuyichao
Copy link
Contributor

the behavior is not intended

Yes, the behavior is clearly not intended. I'm just not sure if the intended behavior is 0.6 behavior or error. I think a consistent rule could be made to support this in an inference independent way though I also don't like making dispatch barrier affecting the semantics this much....

@JeffBezanson JeffBezanson added error handling Handling of exceptions by Julia or the user compiler:codegen Generation of LLVM IR and native code maths Mathematical functions labels Sep 27, 2018
pkofod added a commit to pkofod/julia that referenced this issue Sep 27, 2018
__ldexp_exp(f) requires the second argument to be an Int32, so we might as well hard-code that here. It also seems that the old specification is actually not valid, or might have unexpected results, so we should just go with Int32, see JuliaLang#29400 .
KristofferC pushed a commit that referenced this issue Sep 29, 2018
__ldexp_exp(f) requires the second argument to be an Int32, so we might as well hard-code that here. It also seems that the old specification is actually not valid, or might have unexpected results, so we should just go with Int32, see #29400 .
KristofferC pushed a commit that referenced this issue Sep 30, 2018
__ldexp_exp(f) requires the second argument to be an Int32, so we might as well hard-code that here. It also seems that the old specification is actually not valid, or might have unexpected results, so we should just go with Int32, see #29400 .

(cherry picked from commit 75f798f)
@vtjnash vtjnash removed the maths Mathematical functions label Oct 18, 2018
@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 18, 2018

Strange. Codegen is supposed to reject this IR as malformed—I'm not sure why it's not getting properly detected.

KristofferC pushed a commit that referenced this issue Feb 11, 2019
__ldexp_exp(f) requires the second argument to be an Int32, so we might as well hard-code that here. It also seems that the old specification is actually not valid, or might have unexpected results, so we should just go with Int32, see #29400 .

(cherry picked from commit 75f798f)
KristofferC pushed a commit that referenced this issue Feb 20, 2020
__ldexp_exp(f) requires the second argument to be an Int32, so we might as well hard-code that here. It also seems that the old specification is actually not valid, or might have unexpected results, so we should just go with Int32, see #29400 .

(cherry picked from commit 75f798f)
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 1, 2024

Validation correctly implemented by #40947 now

julia> __ldexp_exp(x::Float64, i::T) where T = ccall(("__ldexp_exp", Base.Math.libm), Float64, (Float64, T), x, i)
ERROR: TypeError: in ccall method definition, expected Type, got a value of type TypeVar

@vtjnash vtjnash closed this as completed Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code error handling Handling of exceptions by Julia or the user
Projects
None yet
Development

No branches or pull requests

4 participants