Skip to content

Commit

Permalink
alignment: subtly change meaning of datatype_align (#34473)
Browse files Browse the repository at this point in the history 
Rather than meaning the actual alignment of the object, this now means
the preferred alignment of the object. The actual alignment of any
object is the minimum of this preferred alignment and the alignment
supported by the runtime allocator. This aligns us with how LLVM treats
alignment, and is probably reasonably sensible anyways.

Also tries to audit our existing uses of CreateLoad/CreateStore for
correctness, and upgrade some to include pointer-types.

fixes #32414
  • Loading branch information
@vtjnash
vtjnash committed Feb 4, 2020
1 parent 9464069 commit 268ac24
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 72 deletions.
14 changes: 7 additions & 7 deletions base/reflection.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ struct DataTypeLayout
nfields::UInt32
npointers::UInt32
firstptr::Int32
alignment::UInt32
# alignment : 9;
alignment::UInt16
flags::UInt16
# haspadding : 1;
# fielddesc_type : 2;
end
Expand All @@ -335,7 +335,7 @@ function datatype_alignment(dt::DataType)
@_pure_meta
dt.layout == C_NULL && throw(UndefRefError())
alignment = unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).alignment
return Int(alignment & 0x1FF)
return Int(alignment)
end

# amount of total space taken by T when stored in a container
Expand Down Expand Up @@ -368,8 +368,8 @@ Can be called on any `isconcretetype`.
function datatype_haspadding(dt::DataType)
@_pure_meta
dt.layout == C_NULL && throw(UndefRefError())
alignment = unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).alignment
return (alignment >> 9) & 1 == 1
flags = unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).flags
return flags & 1 == 1
end

"""
Expand Down Expand Up @@ -397,8 +397,8 @@ See also [`fieldoffset`](@ref).
function datatype_fielddesc_type(dt::DataType)
@_pure_meta
dt.layout == C_NULL && throw(UndefRefError())
alignment = unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).alignment
return (alignment >> 10) & 3
flags = unsafe_load(convert(Ptr{DataTypeLayout}, dt.layout)).flags
return (flags >> 1) & 3
end

"""
Expand Down
4 changes: 2 additions & 2 deletions src/array.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ JL_DLLEXPORT jl_array_t *jl_ptr_to_array_1d(jl_value_t *atype, void *data,
else {
align = elsz = sizeof(void*);
}
if (((uintptr_t)data) & (align - 1))
if (((uintptr_t)data) & ((align > JL_HEAP_ALIGNMENT ? JL_HEAP_ALIGNMENT : align) - 1))
jl_exceptionf(jl_argumenterror_type,
"unsafe_wrap: pointer %p is not properly aligned to %u bytes", data, align);

Expand Down Expand Up @@ -385,7 +385,7 @@ JL_DLLEXPORT jl_array_t *jl_ptr_to_array(jl_value_t *atype, void *data,
else {
align = elsz = sizeof(void*);
}
if (((uintptr_t)data) & (align - 1))
if (((uintptr_t)data) & ((align > JL_HEAP_ALIGNMENT ? JL_HEAP_ALIGNMENT : align) - 1))
jl_exceptionf(jl_argumenterror_type,
"unsafe_wrap: pointer %p is not properly aligned to %u bytes", data, align);

Expand Down
5 changes: 3 additions & 2 deletions src/ccall.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ static Value *llvm_type_rewrite(
from = emit_static_alloca(ctx, from_type);
to = emit_bitcast(ctx, from, target_type->getPointerTo());
}
// XXX: deal with possible alignment issues
ctx.builder.CreateStore(v, from);
return ctx.builder.CreateLoad(to);
}
Expand Down Expand Up @@ -514,7 +515,7 @@ static Value *julia_to_native(
tbaa_decorate(jvinfo.tbaa, ctx.builder.CreateStore(emit_unbox(ctx, to, jvinfo, jlto), slot));
}
else {
emit_memcpy(ctx, slot, jvinfo.tbaa, jvinfo, jl_datatype_size(jlto), jl_datatype_align(jlto));
emit_memcpy(ctx, slot, jvinfo.tbaa, jvinfo, jl_datatype_size(jlto), julia_alignment(jlto));
}
return slot;
}
Expand Down Expand Up @@ -1945,7 +1946,7 @@ jl_cgval_t function_sig_t::emit_a_ccall(
assert(rtsz > 0);
Value *strct = emit_allocobj(ctx, rtsz, runtime_bt);
MDNode *tbaa = jl_is_mutable(rt) ? tbaa_mutab : tbaa_immut;
int boxalign = jl_datatype_align(rt);
int boxalign = julia_alignment(rt);
// copy the data from the return value to the new struct
const DataLayout &DL = jl_data_layout;
auto resultTy = result->getType();
Expand Down
27 changes: 17 additions & 10 deletions src/cgutils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,8 @@ static unsigned julia_alignment(jl_value_t *jt)
}
assert(jl_is_datatype(jt) && ((jl_datatype_t*)jt)->layout);
unsigned alignment = jl_datatype_align(jt);
assert(alignment <= JL_HEAP_ALIGNMENT);
assert(JL_HEAP_ALIGNMENT % alignment == 0);
if (alignment > JL_HEAP_ALIGNMENT)
return JL_HEAP_ALIGNMENT;
return alignment;
}

Expand Down Expand Up @@ -594,7 +594,7 @@ static unsigned jl_field_align(jl_datatype_t *dt, size_t i)
unsigned al = jl_field_offset(dt, i);
al |= 16;
al &= -al;
return std::min(al, jl_datatype_align(dt));
return std::min({al, (unsigned)jl_datatype_align(dt), (unsigned)JL_HEAP_ALIGNMENT});
}

static Type *julia_struct_to_llvm(jl_value_t *jt, jl_unionall_t *ua, bool *isboxed, bool llvmcall)
Expand Down Expand Up @@ -636,7 +636,6 @@ static Type *julia_struct_to_llvm(jl_value_t *jt, jl_unionall_t *ua, bool *isbox
if (jst->layout) {
assert(isptr == jl_field_isptr(jst, i));
assert((isptr ? sizeof(void*) : fsz + jl_is_uniontype(ty)) == jl_field_size(jst, i));
assert(al <= jl_field_align(jst, i));
}
Type *lty;
if (isptr) {
Expand All @@ -647,8 +646,15 @@ static Type *julia_struct_to_llvm(jl_value_t *jt, jl_unionall_t *ua, bool *isbox
lty = T_int8;
}
else if (jl_is_uniontype(ty)) {
// pick an Integer type size such that alignment will be correct
// and always end with an Int8 (selector byte)
// pick an Integer type size such that alignment will generally be correct,
// and always end with an Int8 (selector byte).
// We may need to insert padding first to get to the right offset
if (al > MAX_ALIGN) {
Type *AlignmentType = ArrayType::get(VectorType::get(T_int8, al), 0);
latypes.push_back(AlignmentType);
al = MAX_ALIGN;
}
assert(al <= jl_field_align(jst, i));
Type *AlignmentType = IntegerType::get(jl_LLVMContext, 8 * al);
unsigned NumATy = fsz / al;
unsigned remainder = fsz % al;
Expand Down Expand Up @@ -1212,7 +1218,7 @@ static Value *emit_isconcrete(jl_codectx_t &ctx, Value *typ)
{
Value *isconcrete;
isconcrete = ctx.builder.CreateConstInBoundsGEP1_32(T_int8, emit_bitcast(ctx, decay_derived(typ), T_pint8), offsetof(jl_datatype_t, isconcretetype));
isconcrete = ctx.builder.CreateLoad(isconcrete, tbaa_const);
isconcrete = ctx.builder.CreateLoad(T_int8, isconcrete, tbaa_const);
isconcrete = ctx.builder.CreateTrunc(isconcrete, T_int1);
return isconcrete;
}
Expand Down Expand Up @@ -1336,7 +1342,7 @@ static jl_cgval_t typed_load(jl_codectx_t &ctx, Value *ptr, Value *idx_0based, j
//}
//else {
load = ctx.builder.CreateAlignedLoad(data,
isboxed || alignment ? alignment : julia_alignment(jltype),
isboxed || alignment ? alignment : julia_alignment(jltype),
false);
if (aliasscope)
load->setMetadata("alias.scope", aliasscope);
Expand Down Expand Up @@ -2434,7 +2440,8 @@ static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo)
static void emit_unionmove(jl_codectx_t &ctx, Value *dest, MDNode *tbaa_dst, const jl_cgval_t &src, Value *skip, bool isVolatile=false)
{
if (AllocaInst *ai = dyn_cast<AllocaInst>(dest))
ctx.builder.CreateStore(UndefValue::get(ai->getAllocatedType()), ai);
// TODO: make this a lifetime_end & dereferencable annotation?
ctx.builder.CreateAlignedStore(UndefValue::get(ai->getAllocatedType()), ai, ai->getAlignment());
if (jl_is_concrete_type(src.typ) || src.constant) {
jl_value_t *typ = src.constant ? jl_typeof(src.constant) : src.typ;
Type *store_ty = julia_type_to_llvm(typ);
Expand Down Expand Up @@ -2694,7 +2701,7 @@ static jl_cgval_t emit_new_struct(jl_codectx_t &ctx, jl_value_t *ty, size_t narg
if (jl_field_isptr(sty, i)) {
fval = boxed(ctx, fval_info);
if (!init_as_value)
tbaa_decorate(tbaa_stack, ctx.builder.CreateStore(fval, dest));
tbaa_decorate(tbaa_stack, ctx.builder.CreateAlignedStore(fval, dest, jl_field_align(sty, i)));
}
else if (jl_is_uniontype(jtype)) {
// compute tindex from rhs
Expand Down
Loading

0 comments on commit 268ac24

Please sign in to comment.