-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
RFC: change lowering of ccall cconvert arguments and add Ref{T} type #9986
Changes from all commits
51fc652
c6ee2f9
5202f8e
571096b
9edf5a4
721f3e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
*.do | ||
*.o | ||
*.obj | ||
*.so | ||
*.dylib | ||
*.dSYM | ||
*.jl.cov | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,16 +52,15 @@ convert{T}(::Type{(T...)}, x::Tuple) = cnvt_all(T, x...) | |
cnvt_all(T) = () | ||
cnvt_all(T, x, rest...) = tuple(convert(T,x), cnvt_all(T, rest...)...) | ||
|
||
|
||
ptr_arg_convert{T}(::Type{Ptr{T}}, x) = convert(T, x) | ||
ptr_arg_convert(::Type{Ptr{Void}}, x) = x | ||
|
||
# conversion used by ccall | ||
cconvert(T, x) = convert(T, x) | ||
# use the code in ccall.cpp to safely allocate temporary pointer arrays | ||
cconvert{T}(::Type{Ptr{Ptr{T}}}, a::Array) = a | ||
# convert strings to ByteString to pass as pointers | ||
cconvert{P<:Union(Int8,UInt8)}(::Type{Ptr{P}}, s::AbstractString) = bytestring(s) | ||
# conversions used by ccall | ||
ptr_arg_cconvert{T}(::Type{Ptr{T}}, x) = cconvert(T, x) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we eventually replace these with
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. ptr_arg should eventually go away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will you be able to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes (Already implemented here) Or pass 1 to an argument of type Ref{Int16} |
||
ptr_arg_unsafe_convert{T}(::Type{Ptr{T}}, x) = unsafe_convert(T, x) | ||
ptr_arg_unsafe_convert(::Type{Ptr{Void}}, x) = x | ||
|
||
cconvert(T::Type, x) = convert(T, x) # do the conversion eagerly in most cases | ||
cconvert{P<:Ptr}(::Type{P}, x) = x # but defer the conversion to Ptr to unsafe_convert | ||
unsafe_convert{T}(::Type{T}, x::T) = x # unsafe_convert (like convert) defaults to assuming the convert occurred | ||
unsafe_convert{P<:Ptr}(::Type{P}, x::Ptr) = convert(P, x) | ||
|
||
reinterpret{T,S}(::Type{T}, x::S) = box(T,unbox(S,x)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,7 +112,20 @@ t_func[fpiseq] = (2, 2, cmp_tfunc) | |
t_func[fpislt] = (2, 2, cmp_tfunc) | ||
t_func[nan_dom_err] = (2, 2, (a, b)->a) | ||
t_func[eval(Core.Intrinsics,:ccall)] = | ||
(3, Inf, (fptr, rt, at, a...)->(isType(rt) ? rt.parameters[1] : Any)) | ||
(3, Inf, function(fptr, rt, at, a...) | ||
if !isType(rt) | ||
return Any | ||
end | ||
t = rt.parameters[1] | ||
if isa(t,DataType) && is((t::DataType).name,Ref.name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does it mean if a ccall returns a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See gist or doc updates on the jn/ccall3 PR. Some of these updates are primarily only meaningful in that context |
||
t = t.parameters[1] | ||
if is(t,Any) | ||
return Union() # a return type of Box{Any} is invalid | ||
end | ||
return t | ||
end | ||
return t | ||
end) | ||
t_func[eval(Core.Intrinsics,:llvmcall)] = | ||
(3, Inf, (fptr, rt, at, a...)->(isType(rt) ? rt.parameters[1] : | ||
isa(rt,Tuple) ? map(x->x.parameters[1],rt) : Any)) | ||
|
@@ -2119,8 +2132,7 @@ function is_pure_builtin(f) | |
f === Intrinsics.pointerset || # this one is never effect-free | ||
f === Intrinsics.ccall || # this one is never effect-free | ||
f === Intrinsics.llvmcall || # this one is never effect-free | ||
f === Intrinsics.jl_alloca || # this one is volatile, TODO: possibly also effect-free? | ||
f === Intrinsics.pointertoref) # this one is volatile | ||
f === Intrinsics.jl_alloca) | ||
return true | ||
end | ||
end | ||
|
@@ -2364,7 +2376,7 @@ function inlineable(f::ANY, e::Expr, atypes::Tuple, sv::StaticVarInfo, enclosing | |
if incompletematch | ||
cost *= 4 | ||
end | ||
if is(f, next) || is(f, done) | ||
if is(f, next) || is(f, done) || is(f, unsafe_convert) || is(f, cconvert) | ||
cost /= 4 | ||
end | ||
if !inline_worthy(body, cost) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ function info{T}( | |
regex::Ptr{Void}, | ||
extra::Ptr{Void}, what::Integer, ::Type{T} | ||
) | ||
buf = Array(UInt8,sizeof(T)) | ||
buf = zeros(UInt8,sizeof(T)) | ||
ret = ccall((:pcre_fullinfo, :libpcre), Int32, | ||
(Ptr{Void}, Ptr{Void}, Int32, Ptr{UInt8}), | ||
regex, extra, what, buf) | ||
|
@@ -71,7 +71,7 @@ function info{T}( | |
end | ||
|
||
function config{T}(what::Integer, ::Type{T}) | ||
buf = Array(UInt8, sizeof(T)) | ||
buf = zeros(UInt8, sizeof(T)) | ||
ret = ccall((:pcre_config, :libpcre), Int32, | ||
(Int32, Ptr{UInt8}), | ||
what, buf) | ||
|
@@ -84,7 +84,8 @@ end | |
|
||
function compile(pattern::AbstractString, options::Integer) | ||
errstr = Array(Ptr{UInt8},1) | ||
erroff = Array(Int32,1) | ||
errstr[1] = C_NULL | ||
erroff = zeros(Int32,1) | ||
re_ptr = ccall((:pcre_compile, :libpcre), Ptr{Void}, | ||
(Ptr{UInt8}, Int32, Ptr{Ptr{UInt8}}, Ptr{Int32}, Ptr{UInt8}), | ||
pattern, options, errstr, erroff, C_NULL) | ||
|
@@ -100,6 +101,7 @@ end | |
function study(regex::Ptr{Void}, options::Integer) | ||
# NOTE: options should always be zero in current PCRE | ||
errstr = Array(Ptr{UInt8},1) | ||
errstr[1] = C_NULL | ||
extra = ccall((:pcre_study, :libpcre), Ptr{Void}, | ||
(Ptr{Void}, Int32, Ptr{Ptr{UInt8}}), | ||
regex, options, errstr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like bug fixes in this file? Could you apply these to master? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not exactly a bug. Pcre should zero this field, but I had errors in the way I lowered ccall and this helped me get further |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch this is kind of expensive. Especially since in common cases
cconvert_gcroot
is identity. Seems like a pretty big performance hit over what we have now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in common cases,
cconvert_gcroot
is identity, butcconvert
might be arbitrarily expensive. this just moves some of that work early and ensurescconvert
is simple.this is also significantly more general (and better rooted) than we what we have now. the old code could handle turning arrays of strings into pointers to pointers, but i don't entirely know what it would do given anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we were missing any roots, since before we allocated these objects in the temp_arg_area.