Skip to content

Commit

Permalink
splatnew: ensure performance of struct users of it (JuliaLang#31550)
Browse files Browse the repository at this point in the history
Types which permit nargs < nfields in their constructor are required by correctness
to take a notable performance hit. Since currently nobody is using currently that
ability (NamedTuples, for example, want it to be statically disallowed),
ensure that splatnew respects that we must have nargs >= ninitialized
in the most performance-preserving way we can.
  • Loading branch information
vtjnash committed Apr 4, 2019
1 parent 80e4862 commit 084e8c7
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 19 deletions.
1 change: 0 additions & 1 deletion base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -944,7 +944,6 @@ function abstract_eval(@nospecialize(e), vtypes::VarTable, sv::InferenceState)
end
elseif e.head === :splatnew
t = instanceof_tfunc(abstract_eval(e.args[1], vtypes, sv))[1]
# TODO: improve
elseif e.head === :&
abstract_eval(e.args[1], vtypes, sv)
t = Any
Expand Down
7 changes: 5 additions & 2 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ function is_valid_type_for_apply_rewrite(@nospecialize(typ), params::Params)
end
end

function inline_splatnew!(ir::IRCode, idx)
function inline_splatnew!(ir::IRCode, idx::Int)
stmt = ir.stmts[idx]
ty = ir.types[idx]
nf = nfields_tfunc(ty)
Expand All @@ -830,7 +830,9 @@ function inline_splatnew!(ir::IRCode, idx)
tup = eargs[2]
tt = argextype(tup, ir, ir.sptypes)
tnf = nfields_tfunc(tt)
if tnf isa Const && tnf.val <= nf.val
# TODO: hoisting this tnf.val == nf.val check into codegen
# would enable us to almost always do this transform
if tnf isa Const && tnf.val == nf.val
n = tnf.val
new_argexprs = Any[eargs[1]]
for j = 1:n
Expand All @@ -843,6 +845,7 @@ function inline_splatnew!(ir::IRCode, idx)
stmt.args = new_argexprs
end
end
nothing
end

function call_sig(ir::IRCode, stmt::Expr)
Expand Down
2 changes: 2 additions & 0 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4153,6 +4153,8 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaval)
Value *typ = boxed(ctx, argv[0]);
Value *tup = boxed(ctx, argv[1]);
Value *val = ctx.builder.CreateCall(prepare_call(jlsplatnew_func), { typ, tup });
// temporarily mark as `Any`, expecting `emit_ssaval_assign` to update
// it to the inferred type.
return mark_julia_type(ctx, val, true, (jl_value_t*)jl_any_type);
}
else if (head == exc_sym) {
Expand Down
10 changes: 4 additions & 6 deletions src/datatype.c
Original file line number Diff line number Diff line change
Expand Up @@ -848,13 +848,12 @@ JL_DLLEXPORT jl_value_t *jl_new_structt(jl_datatype_t *type, jl_value_t *tup)
jl_ptls_t ptls = jl_get_ptls_states();
if (!jl_is_tuple(tup))
jl_type_error("new", (jl_value_t*)jl_tuple_type, tup);
size_t na = jl_nfields(tup);
size_t nargs = jl_nfields(tup);
size_t nf = jl_datatype_nfields(type);
if (na > nf)
jl_too_many_args("new", nf);
JL_NARGS(new, nf, nf);
if (type->instance != NULL) {
jl_datatype_t *tupt = (jl_datatype_t*)jl_typeof(tup);
for (size_t i = 0; i < na; i++) {
for (size_t i = 0; i < nargs; i++) {
jl_value_t *ft = jl_field_type(type, i);
jl_value_t *et = jl_field_type(tupt, i);
assert(jl_is_concrete_type(ft) && jl_is_concrete_type(et));
Expand All @@ -868,14 +867,13 @@ JL_DLLEXPORT jl_value_t *jl_new_structt(jl_datatype_t *type, jl_value_t *tup)
jl_value_t *jv = jl_gc_alloc(ptls, jl_datatype_size(type), type);
jl_value_t *fi = NULL;
JL_GC_PUSH2(&jv, &fi);
for (size_t i = 0; i < na; i++) {
for (size_t i = 0; i < nargs; i++) {
jl_value_t *ft = jl_field_type(type, i);
fi = jl_get_nth_field(tup, i);
if (!jl_isa(fi, ft))
jl_type_error("new", ft, fi);
jl_set_nth_field(jv, i, fi);
}
init_struct_tail(type, jv, na);
JL_GC_POP();
return jv;
}
Expand Down
17 changes: 7 additions & 10 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -6854,31 +6854,28 @@ g29828() = 2::Any[String][1]
struct SplatNew{T}
x::Int8
y::T
SplatNew{T}(args...) where {T} = new(0,args...,1)
SplatNew{T}(args...) where {T} = new(0, args..., 1)
SplatNew(args...) = new{Float32}(args...)
SplatNew{Any}(args...) = new(args...)
SplatNew{Tuple{Int16}}(args...) = new([2]..., args...)
SplatNew{Int8}() = new(1,2,3)
SplatNew{Int8}() = new(1, 2, 3)
end
let x = SplatNew{Int16}()
@test x.x === Int8(0)
@test x.y === Int16(1)
end
@test_throws ArgumentError SplatNew{Int16}(1)
let x = SplatNew(3,2)
@test_throws ArgumentError("new: too many arguments (expected 2)") SplatNew{Int16}(1)
let x = SplatNew(3, 2)
@test x.x === Int8(3)
@test x.y === 2.0f0
end
@test_throws ArgumentError SplatNew(1,2,3)
let x = SplatNew{Any}(1)
@test x.x === Int8(1)
@test !isdefined(x, :y)
end
@test_throws ArgumentError("new: too many arguments (expected 2)") SplatNew(1, 2, 3)
@test_throws ArgumentError("new: too few arguments (expected 2)") SplatNew{Any}(1)
let x = SplatNew{Tuple{Int16}}((1,))
@test x.x === Int8(2)
@test x.y === (Int16(1),)
end
@test_throws ArgumentError SplatNew{Int8}()
@test_throws ArgumentError("new: too many arguments (expected 2)") SplatNew{Int8}()

# Issue #31357 - Missed assignment in nested try/catch
function foo31357(b::Bool)
Expand Down

0 comments on commit 084e8c7

Please sign in to comment.