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.

Sign up for GitHub

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

Jump to bottom

codegen: complete handling for partial-layout objects #41438

Merged
merged 1 commit into from
Jul 4, 2021
Merged

codegen: complete handling for partial-layout objects #41438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ccall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ static Value *julia_to_native(
assert(!byRef); // don't expect any ABI to pass pointers by pointer
return boxed(ctx, jvinfo);
}
assert(jl_is_datatype(jlto) && julia_struct_has_layout((jl_datatype_t*)jlto));
assert(jl_is_datatype(jlto) && jl_struct_try_layout((jl_datatype_t*)jlto));

typeassert_input(ctx, jvinfo, jlto, jlto_env, argn);
if (!byRef)
Expand Down
20 changes: 7 additions & 13 deletions src/cgutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ static size_t dereferenceable_size(jl_value_t *jt)
// Array has at least this much data
return sizeof(jl_array_t);
}
else if (jl_is_datatype(jt) && ((jl_datatype_t*)jt)->layout) {
else if (jl_is_datatype(jt) && jl_struct_try_layout((jl_datatype_t*)jt)) {
return jl_datatype_size(jt);
}
return 0;
Expand All @@ -339,7 +339,7 @@ static unsigned julia_alignment(jl_value_t *jt)
// and this is the guarantee we have for the GC bits
return 16;
}
assert(jl_is_datatype(jt) && ((jl_datatype_t*)jt)->layout);
assert(jl_is_datatype(jt) && jl_struct_try_layout((jl_datatype_t*)jt));
unsigned alignment = jl_datatype_align(jt);
if (alignment > JL_HEAP_ALIGNMENT)
return JL_HEAP_ALIGNMENT;
Expand Down Expand Up @@ -555,16 +555,10 @@ static Type *bitstype_to_llvm(jl_value_t *bt, bool llvmcall = false)
}

static bool jl_type_hasptr(jl_value_t* typ)
{ // assumes that jl_stored_inline(typ) is true
{ // assumes that jl_stored_inline(typ) is true (and therefore that layout is defined)
return jl_is_datatype(typ) && ((jl_datatype_t*)typ)->layout->npointers > 0;
}

// return whether all concrete subtypes of this type have the same layout
static bool julia_struct_has_layout(jl_datatype_t *dt)
{
return dt->layout || jl_has_fixed_layout(dt);
}

static unsigned jl_field_align(jl_datatype_t *dt, size_t i)
{
unsigned al = jl_field_offset(dt, i);
Expand All @@ -588,11 +582,8 @@ static Type *_julia_struct_to_llvm(jl_codegen_params_t *ctx, jl_value_t *jt, boo
bool isTuple = jl_is_tuple_type(jt);
jl_svec_t *ftypes = jl_get_fieldtypes(jst);
size_t i, ntypes = jl_svec_len(ftypes);
if (!julia_struct_has_layout(jst))
if (!jl_struct_try_layout(jst))
return NULL; // caller should have checked jl_type_mappable_to_c already, but we'll be nice
if (jst->layout == NULL)
jl_compute_field_offsets(jst);
assert(jst->layout);
if (ntypes == 0 || jl_datatype_nbits(jst) == 0)
return T_void;
Type *_struct_decl = NULL;
Expand Down Expand Up @@ -1904,6 +1895,9 @@ static bool emit_getfield_unknownidx(jl_codectx_t &ctx,
return true;
}
if (nfields == 1) {
if (jl_has_free_typevars(jl_field_type(stt, 0))) {
return false;
}
(void)idx0();
*ret = emit_getfield_knownidx(ctx, strct, 0, stt, order);
return true;
Expand Down
22 changes: 11 additions & 11 deletions src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,12 +933,12 @@ static MDNode *best_tbaa(jl_value_t *jt) {
// note that this includes jl_isbits, although codegen should work regardless
static bool jl_is_concrete_immutable(jl_value_t* t)
{
return jl_is_immutable_datatype(t) && ((jl_datatype_t*)t)->layout;
return jl_is_immutable_datatype(t) && ((jl_datatype_t*)t)->isconcretetype;
}

static bool jl_is_pointerfree(jl_value_t* t)
{
if (!jl_is_immutable_datatype(t))
if (!jl_is_concrete_immutable(t))
return 0;
const jl_datatype_layout_t *layout = ((jl_datatype_t*)t)->layout;
return layout && layout->npointers == 0;
Expand Down Expand Up @@ -3033,9 +3033,9 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
return true;
}

if (jl_is_datatype(utt) && utt->layout) {
if (jl_is_datatype(utt) && jl_struct_try_layout(utt)) {
ssize_t idx = jl_field_index(utt, name, 0);
if (idx != -1) {
if (idx != -1 && !jl_has_free_typevars(jl_field_type(utt, idx))) {
*ret = emit_getfield_knownidx(ctx, obj, idx, utt, order);
return true;
}
Expand Down Expand Up @@ -3063,14 +3063,16 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
}

if (jl_is_datatype(utt)) {
if (jl_is_structtype(utt) && utt->layout) {
if (jl_struct_try_layout(utt)) {
size_t nfields = jl_datatype_nfields(utt);
// integer index
size_t idx;
if (fld.constant && (idx = jl_unbox_long(fld.constant) - 1) < nfields) {
// known index
*ret = emit_getfield_knownidx(ctx, obj, idx, utt, order);
return true;
if (!jl_has_free_typevars(jl_field_type(utt, idx))) {
// known index
*ret = emit_getfield_knownidx(ctx, obj, idx, utt, order);
return true;
}
}
else {
// unknown index
Expand Down Expand Up @@ -3115,8 +3117,6 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
}
}
}
// TODO: attempt better codegen for approximate types, if the types
// and offsets of some fields are independent of parameters.
// TODO: generic getfield func with more efficient calling convention
return false;
}
Expand Down Expand Up @@ -3155,7 +3155,7 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
}

jl_datatype_t *uty = (jl_datatype_t*)jl_unwrap_unionall(obj.typ);
if (jl_is_structtype(uty) && uty->layout) {
if (jl_is_datatype(uty) && jl_struct_try_layout(uty)) {
ssize_t idx = -1;
if (fld.constant && fld.typ == (jl_value_t*)jl_symbol_type) {
idx = jl_field_index(uty, (jl_sym_t*)fld.constant, 0);
Expand Down
41 changes: 26 additions & 15 deletions src/datatype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,27 +220,36 @@ unsigned jl_special_vector_alignment(size_t nfields, jl_value_t *t)
return next_power_of_two(size);
}

STATIC_INLINE int jl_is_datatype_make_singleton(jl_datatype_t *d)
STATIC_INLINE int jl_is_datatype_make_singleton(jl_datatype_t *d) JL_NOTSAFEPOINT
{
return (!d->name->abstract && jl_datatype_size(d) == 0 && d != jl_symbol_type && d->name != jl_array_typename &&
d->isconcretetype && !d->name->mutabl);
}

STATIC_INLINE void jl_maybe_allocate_singleton_instance(jl_datatype_t *st)
STATIC_INLINE void jl_maybe_allocate_singleton_instance(jl_datatype_t *st) JL_NOTSAFEPOINT
{
if (jl_is_datatype_make_singleton(st)) {
// It's possible for st to already have an ->instance if it was redefined
if (!st->instance) {
jl_task_t *ct = jl_current_task;
st->instance = jl_gc_alloc(ct->ptls, 0, st);
jl_gc_wb(st, st->instance);
}
if (!st->instance)
st->instance = jl_gc_permobj(0, st);
}
}

// return whether all concrete subtypes of this type have the same layout
int jl_struct_try_layout(jl_datatype_t *dt)
{
if (dt->layout)
return 1;
else if (!jl_has_fixed_layout(dt))
return 0;
jl_compute_field_offsets(dt);
assert(dt->layout);
return 1;
}

int jl_datatype_isinlinealloc(jl_datatype_t *ty, int pointerfree) JL_NOTSAFEPOINT
{
if (ty->name->mayinlinealloc && ty->layout) {
if (ty->name->mayinlinealloc && (ty->isconcretetype || ((jl_datatype_t*)jl_unwrap_unionall(ty->name->wrapper))->layout)) { // TODO: use jl_struct_try_layout(dt) (but it is a safepoint)
if (ty->layout->npointers > 0) {
if (pointerfree)
return 0;
Expand Down Expand Up @@ -348,28 +357,31 @@ void jl_compute_field_offsets(jl_datatype_t *st)
if (st->name->wrapper == NULL)
return; // we got called too early--we'll be back
jl_datatype_t *w = (jl_datatype_t*)jl_unwrap_unionall(st->name->wrapper);
assert(st->types && w->types);
size_t i, nfields = jl_svec_len(st->types);
assert(st->name->n_uninitialized <= nfields);
if (st == w && st->layout) {
// this check allows us to force re-computation of the layout for some types during init
st->layout = NULL;
st->size = 0;
st->zeroinit = 0;
st->has_concrete_subtype = 1;
}
int isbitstype = st->isconcretetype && st->name->mayinlinealloc;
// If layout doesn't depend on type parameters, it's stored in st->name->wrapper
// and reused by all subtypes.
if (w->layout) {
st->layout = w->layout;
st->size = w->size;
st->zeroinit = w->zeroinit;
st->has_concrete_subtype = w->has_concrete_subtype;
if (jl_is_layout_opaque(st->layout)) { // e.g. jl_array_typename
return;
if (!jl_is_layout_opaque(st->layout)) { // e.g. jl_array_typename
st->isbitstype = isbitstype && st->layout->npointers == 0;
jl_maybe_allocate_singleton_instance(st);
}
return;
}
else if (nfields == 0) {
assert(st->types && w->types);
size_t i, nfields = jl_svec_len(st->types);
assert(st->name->n_uninitialized <= nfields);
if (nfields == 0) {
// if we have no fields, we can trivially skip the rest
if (st == jl_symbol_type || st == jl_string_type) {
// opaque layout - heap-allocated blob
Expand Down Expand Up @@ -404,7 +416,6 @@ void jl_compute_field_offsets(jl_datatype_t *st)
}
}

int isbitstype = st->isconcretetype && st->name->mayinlinealloc;
for (i = 0; isbitstype && i < nfields; i++) {
jl_value_t *fld = jl_field_type(st, i);
isbitstype = jl_isbits(fld);
Expand Down
28 changes: 14 additions & 14 deletions src/jltypes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ int jl_has_fixed_layout(jl_datatype_t *dt)
{
if (dt->layout || dt->isconcretetype)
return 1;
if (jl_is_tuple_type(dt))
if (dt->name->abstract)
return 0;
if (jl_is_tuple_type(dt) || jl_is_namedtuple_type(dt))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since NamedTuples are invariant is this really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are invariant, but they don't behave properly, so yes, this is necessary to avoid crashes

return 0; // TODO: relax more?
jl_svec_t *types = jl_get_fieldtypes(dt);
size_t i, l = jl_svec_len(types);
Expand All @@ -242,9 +244,10 @@ int jl_type_mappable_to_c(jl_value_t *ty)
assert(!jl_is_typevar(ty) && jl_is_type(ty));
if (jl_is_structtype(ty)) {
jl_datatype_t *jst = (jl_datatype_t*)ty;
return jst->layout || jl_has_fixed_layout(jst);
return jl_has_fixed_layout(jst);
}
if (jl_is_tuple_type(jl_unwrap_unionall(ty)))
ty = jl_unwrap_unionall(ty);
if (jl_is_tuple_type(ty) || jl_is_namedtuple_type(ty))
return 0; // TODO: relax some?
return 1; // as boxed or primitive
}
Expand Down Expand Up @@ -1437,7 +1440,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
jl_gc_wb(ndt, ndt->parameters);
ndt->types = NULL; // to be filled in below
if (istuple) {
ndt->types = p;
ndt->types = p; // TODO: this may need to filter out certain types
}
else if (isnamedtuple) {
jl_value_t *names_tup = jl_svecref(p, 0);
Expand All @@ -1463,19 +1466,16 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
jl_gc_wb(ndt, ndt->types);
}
else {
ndt->types = jl_emptysvec;
ndt->types = jl_emptysvec; // XXX: this is essentially always false
}
}
ndt->size = 0;
jl_precompute_memoized_dt(ndt, cacheable);

if (jl_is_primitivetype(dt)) {
ndt->size = dt->size;
ndt->layout = dt->layout;
ndt->isbitstype = ndt->isconcretetype;
}

jl_datatype_t *primarydt = ((jl_datatype_t*)jl_unwrap_unionall(tn->wrapper));
jl_precompute_memoized_dt(ndt, cacheable);
ndt->size = 0;
if (primarydt->layout)
jl_compute_field_offsets(ndt);

if (istuple || isnamedtuple) {
ndt->super = jl_any_type;
}
Expand Down Expand Up @@ -1516,7 +1516,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
// leading to incorrect layouts and data races (#40050: the A{T} should be
// an isbitstype singleton of size 0)
if (cacheable) {
if (!jl_is_primitivetype(dt) && ndt->types != NULL && ndt->isconcretetype) {
if (dt->layout == NULL && !jl_is_primitivetype(dt) && ndt->types != NULL && ndt->isconcretetype) {
jl_compute_field_offsets(ndt);
}
jl_cache_type_(ndt);
Expand Down
3 changes: 2 additions & 1 deletion src/julia_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ STATIC_INLINE jl_gc_tracked_buffer_t *jl_gc_alloc_buf(jl_ptls_t ptls, size_t sz)
return jl_gc_alloc(ptls, sz, (void*)jl_buff_tag);
}

STATIC_INLINE jl_value_t *jl_gc_permobj(size_t sz, void *ty)
STATIC_INLINE jl_value_t *jl_gc_permobj(size_t sz, void *ty) JL_NOTSAFEPOINT
{
const size_t allocsz = sz + sizeof(jl_taggedvalue_t);
unsigned align = (sz == 0 ? sizeof(void*) : (allocsz <= sizeof(void*) * 2 ?
Expand Down Expand Up @@ -539,6 +539,7 @@ jl_datatype_t *jl_mk_builtin_func(jl_datatype_t *dt, const char *name, jl_fptr_a
int jl_obviously_unequal(jl_value_t *a, jl_value_t *b);
JL_DLLEXPORT jl_array_t *jl_find_free_typevars(jl_value_t *v);
int jl_has_fixed_layout(jl_datatype_t *t);
int jl_struct_try_layout(jl_datatype_t *dt);
int jl_type_mappable_to_c(jl_value_t *ty);
jl_svec_t *jl_outer_unionall_vars(jl_value_t *u);
jl_value_t *jl_type_intersection_env_s(jl_value_t *a, jl_value_t *b, jl_svec_t **penv, int *issubty);
Expand Down
13 changes: 13 additions & 0 deletions test/compiler/codegen.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,3 +580,16 @@ g40612(a, b) = a[]|a[] === b[]|b[]
@test g40612(Union{Bool,Missing}[missing], Union{Bool,Missing}[missing])
@test g40612(Union{Bool,Missing}[true], Union{Bool,Missing}[true])
@test g40612(Union{Bool,Missing}[false], Union{Bool,Missing}[false])

# issue #41438
struct A41438{T}
x::Ptr{T}
end
struct B41438{T}
x::T
end
f41438(y) = y[].x
@test A41438.body.layout != C_NULL
@test B41438.body.layout === C_NULL
@test f41438(Ref{A41438}(A41438(C_NULL))) === C_NULL
@test f41438(Ref{B41438}(B41438(C_NULL))) === C_NULL