From 8339344963f8bac70956a13ae3b21a7099f88d35 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Mon, 20 Jul 2020 15:57:36 -0400 Subject: [PATCH] Fix correctness issues in sizeof tfunc (#36727) The `Core.sizeof` function can take either a value or a type, which can be a bit confusing in the tfunc, because the tfunc operates on the types of the values passed to the builtin. In particular, we were incorrectly returning Const(16) (on 64bit) for sizeof_tfunc(UnionAll), because it was treating it the asme as `sizeof_tfunc(Const(UnionAll))`. Try to clean that up as well as fixing a similar confusion in the nothrow version of this function. Lastly, we had a similar fast path in codegen, which would try to inline the actual size for a constant datatype argument. For codegen, just rm that whole code path, since it should have no more information than inference and figuring it out in inference exposes strictly more optimization opportunities. Fixes #36710 --- base/array.jl | 8 +++---- base/compiler/tfuncs.jl | 49 +++++++++++++++++++++++++++++++------- base/reflection.jl | 27 +++++++++++++++++---- src/codegen.cpp | 15 ------------ test/compiler/inference.jl | 16 ++++++++++--- 5 files changed, 79 insertions(+), 36 deletions(-) diff --git a/base/array.jl b/base/array.jl index f8de513cc60e8d..e6d64c313c16df 100644 --- a/base/array.jl +++ b/base/array.jl @@ -211,11 +211,9 @@ julia> Base.bitsunionsize(Union{Float64, UInt8, Int128}) ``` """ function bitsunionsize(u::Union) - sz = Ref{Csize_t}(0) - algn = Ref{Csize_t}(0) - isunboxed = ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), u, sz, algn) - @assert isunboxed != Cint(0) - return sz[] + isinline, sz, _ = uniontype_layout(u) + @assert isinline + return sz end length(a::Array) = arraylen(a) diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl index a6007e0cfac62b..ec41e46ffa96fc 100644 --- a/base/compiler/tfuncs.jl +++ b/base/compiler/tfuncs.jl @@ -302,16 +302,34 @@ function sizeof_nothrow(@nospecialize(x)) end elseif isa(x, Conditional) return true - else - x = widenconst(x) end if isa(x, Union) return sizeof_nothrow(x.a) && sizeof_nothrow(x.b) end - isconstType(x) && (x = x.parameters[1]) # since sizeof(typeof(x)) == sizeof(x) - x === DataType && return false - return isconcretetype(x) || isprimitivetype(x) + t, exact = instanceof_tfunc(x) + if !exact + # Could always be bottom at runtime, which throws + return false + end + if t !== Bottom + t === DataType && return true + x = t + x = unwrap_unionall(x) + if isa(x, Union) + isinline, sz, _ = uniontype_layout(x) + return isinline + end + isa(x, DataType) || return false + x.layout == C_NULL && return false + (datatype_nfields(x) == 0 && !datatype_pointerfree(x)) && return false + return true + else + x = widenconst(x) + x === DataType && return false + return isconcretetype(x) || isprimitivetype(x) + end end + function _const_sizeof(@nospecialize(x)) # Constant Vector does not have constant size isa(x, Vector) && return Int @@ -330,12 +348,27 @@ function sizeof_tfunc(@nospecialize(x),) isa(x, Const) && return _const_sizeof(x.val) isa(x, Conditional) && return _const_sizeof(Bool) isconstType(x) && return _const_sizeof(x.parameters[1]) - x = widenconst(x) if isa(x, Union) return tmerge(sizeof_tfunc(x.a), sizeof_tfunc(x.b)) end - x !== DataType && isconcretetype(x) && return _const_sizeof(x) - isprimitivetype(x) && return _const_sizeof(x) + # Core.sizeof operates on either a type or a value. First check which + # case we're in. + t, exact = instanceof_tfunc(x) + if t !== Bottom + # The value corresponding to `x` at runtime could be a type. + # Normalize the query to ask about that type. + x = unwrap_unionall(t) + if isa(x, Union) + isinline, sz, _ = uniontype_layout(x) + return isinline ? Const(Int(sz)) : (exact ? Bottom : Int) + end + isa(x, DataType) || return Int + (isconcretetype(x) || isprimitivetype(x)) && return _const_sizeof(x) + else + x = widenconst(x) + x !== DataType && isconcretetype(x) && return _const_sizeof(x) + isprimitivetype(x) && return _const_sizeof(x) + end return Int end add_tfunc(Core.sizeof, 1, 1, sizeof_tfunc, 1) diff --git a/base/reflection.jl b/base/reflection.jl index dc0d753eb41758..927a7d229cf55c 100644 --- a/base/reflection.jl +++ b/base/reflection.jl @@ -347,15 +347,19 @@ function datatype_alignment(dt::DataType) return Int(alignment) end +function uniontype_layout(T::Type) + sz = RefValue{Csize_t}(0) + algn = RefValue{Csize_t}(0) + isinline = ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), T, sz, algn) != 0 + (isinline, sz[], algn[]) +end + # amount of total space taken by T when stored in a container function aligned_sizeof(T::Type) @_pure_meta if isbitsunion(T) - sz = Ref{Csize_t}(0) - algn = Ref{Csize_t}(0) - ccall(:jl_islayout_inline, Cint, (Any, Ptr{Csize_t}, Ptr{Csize_t}), T, sz, algn) - al = algn[] - return (sz[] + al - 1) & -al + _, sz, al = uniontype_layout(T) + return (sz + al - 1) & -al elseif allocatedinline(T) al = datatype_alignment(T) return (Core.sizeof(T) + al - 1) & -al @@ -381,6 +385,19 @@ function datatype_haspadding(dt::DataType) return flags & 1 == 1 end +""" + Base.datatype_nfields(dt::DataType) -> Bool + +Return the number of fields known to this datatype's layout. +Can be called on any `isconcretetype`. +""" +function datatype_nfields(dt::DataType) + @_pure_meta + dt.layout == C_NULL && throw(UndefRefError()) + return unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).nfields +end + + """ Base.datatype_pointerfree(dt::DataType) -> Bool diff --git a/src/codegen.cpp b/src/codegen.cpp index f2082fdb040294..c504920fd08c78 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -3039,21 +3039,6 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, *ret = mark_julia_type(ctx, ctx.builder.CreateMul(len, elsize), false, jl_long_type); return true; } - if (jl_is_type_type((jl_value_t*)sty) && !jl_is_typevar(jl_tparam0(sty))) { - sty = (jl_datatype_t*)jl_tparam0(sty); - } - if (jl_is_datatype(sty) && sty != jl_symbol_type && - sty->name != jl_array_typename && - sty != jl_simplevector_type && sty != jl_string_type && - // exclude DataType, since each DataType has its own size, not sizeof(DataType). - // this is issue #8798 - sty != jl_datatype_type) { - if (jl_is_concrete_type((jl_value_t*)sty) || - (jl_field_names(sty) == jl_emptysvec && jl_datatype_size(sty) > 0)) { - *ret = mark_julia_type(ctx, ConstantInt::get(T_size, jl_datatype_size(sty)), false, jl_long_type); - return true; - } - } } else if (f == jl_builtin_apply_type && nargs > 0) { diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl index 772093f66bdea0..285f944d4bd8c3 100644 --- a/test/compiler/inference.jl +++ b/test/compiler/inference.jl @@ -1400,12 +1400,17 @@ egal_conditional_lattice3(x, y) = x === y + y ? "" : 1 using Core.Compiler: PartialStruct, nfields_tfunc, sizeof_tfunc, sizeof_nothrow @test sizeof_tfunc(Const(Ptr)) === sizeof_tfunc(Union{Ptr, Int, Type{Ptr{Int8}}, Type{Int}}) === Const(Sys.WORD_SIZE รท 8) -@test sizeof_tfunc(Type{Ptr}) === Int +@test sizeof_tfunc(Type{Ptr}) === Const(sizeof(Ptr)) @test sizeof_nothrow(Union{Ptr, Int, Type{Ptr{Int8}}, Type{Int}}) @test sizeof_nothrow(Const(Ptr)) -@test !sizeof_nothrow(Type{Ptr}) -@test !sizeof_nothrow(Type{Union{Ptr{Int}, Int}}) +@test sizeof_nothrow(Type{Ptr}) +@test sizeof_nothrow(Type{Union{Ptr{Int}, Int}}) @test !sizeof_nothrow(Const(Tuple)) +@test !sizeof_nothrow(Type{Vector{Int}}) +@test !sizeof_nothrow(Type{Union{Int, String}}) +@test sizeof_nothrow(String) +@test !sizeof_nothrow(Type{String}) +@test sizeof_tfunc(Type{Union{Int64, Int32}}) == Const(Core.sizeof(Union{Int64, Int32})) let PT = PartialStruct(Tuple{Int64,UInt64}, Any[Const(10, false), UInt64]) @test sizeof_tfunc(PT) === Const(16) @test nfields_tfunc(PT) === Const(2) @@ -2734,3 +2739,8 @@ end f_generator_splat(t::Tuple) = tuple((identity(l) for l in t)...) @test Base.return_types(f_generator_splat, (Tuple{Symbol, Int64, Float64},)) == Any[Tuple{Symbol, Int64, Float64}] + +# Issue #36710 - sizeof(::UnionAll) tfunc correctness +@test (sizeof(Ptr),) == sizeof.((Ptr,)) == sizeof.((Ptr{Cvoid},)) +@test Core.Compiler.sizeof_tfunc(UnionAll) === Int +@test !Core.Compiler.sizeof_nothrow(UnionAll)