Skip to content

Commit 8467772

Browse files
vtjnashvtjnash
committed
codegen: take gc roots (and alloca alignment) more seriously
Due to limitations in the LLVM implementation, we are forced to emit fairly bad code here. But we need to make sure it is still correct with respect to GC rooting. The PR 50833c8 also changed the meaning of haspadding without changing all of the existing users to use the new equivalent flag (haspadding || !isbitsegal), incurring additional breakage here as well and needing more tests for that. Fixes #54720 (cherry picked from commit 8bfef8f)
1 parent e964634 commit 8467772

File tree

2 files changed

+67
-36
lines changed

2 files changed

+67
-36
lines changed

‎src/cgutils.cpp‎

Lines changed: 61 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2092,12 +2092,15 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
20922092
FailOrder = AtomicOrdering::Monotonic;
20932093
unsigned nb = isboxed ? sizeof(void*) : jl_datatype_size(jltype);
20942094
AllocaInst *intcast = nullptr;
2095+
Type *intcast_eltyp = nullptr;
2096+
bool tracked_pointers = isboxed || CountTrackedPointers(elty).count > 0;
20952097
if (!isboxed && Order != AtomicOrdering::NotAtomic && !elty->isIntOrPtrTy()) {
2098+
intcast_eltyp = elty;
2099+
elty = Type::getIntNTy(ctx.builder.getContext(), 8 * nb);
20962100
if (!issetfield) {
20972101
intcast = emit_static_alloca(ctx, elty);
20982102
setName(ctx.emission_context, intcast, "atomic_store_box");
20992103
}
2100-
elty = Type::getIntNTy(ctx.builder.getContext(), 8 * nb);
21012104
}
21022105
Type *realelty = elty;
21032106
if (Order != AtomicOrdering::NotAtomic && isa<IntegerType>(elty)) {
@@ -2106,14 +2109,20 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
21062109
elty = Type::getIntNTy(ctx.builder.getContext(), 8 * nb2);
21072110
}
21082111
Value *r = nullptr;
2109-
if (issetfield || isswapfield || isreplacefield || issetfieldonce) {
2110-
if (isboxed)
2112+
if (issetfield || isswapfield || isreplacefield || issetfieldonce) { // e.g. !ismodifyfield
2113+
assert(isboxed || rhs.typ == jltype);
2114+
if (isboxed) {
21112115
r = boxed(ctx, rhs);
2112-
else if (aliasscope || Order != AtomicOrdering::NotAtomic || CountTrackedPointers(realelty).count) {
2116+
}
2117+
else if (intcast) {
2118+
emit_unbox_store(ctx, rhs, intcast, ctx.tbaa().tbaa_stack, intcast->getAlign());
2119+
r = ctx.builder.CreateLoad(realelty, intcast);
2120+
}
2121+
else if (aliasscope || Order != AtomicOrdering::NotAtomic || tracked_pointers) {
21132122
r = emit_unbox(ctx, realelty, rhs, jltype);
2114-
if (realelty != elty)
2115-
r = ctx.builder.CreateZExt(r, elty);
21162123
}
2124+
if (realelty != elty)
2125+
r = ctx.builder.CreateZExt(r, elty);
21172126
}
21182127
Type *ptrty = PointerType::get(elty, ptr->getType()->getPointerAddressSpace());
21192128
if (ptr->getType() != ptrty)
@@ -2197,7 +2206,14 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
21972206
Current->addIncoming(instr, SkipBB);
21982207
ctx.builder.SetInsertPoint(BB);
21992208
}
2200-
Compare = emit_unbox(ctx, realelty, cmp, jltype);
2209+
cmp = update_julia_type(ctx, cmp, jltype);
2210+
if (intcast) {
2211+
emit_unbox_store(ctx, cmp, intcast, ctx.tbaa().tbaa_stack, intcast->getAlign());
2212+
Compare = ctx.builder.CreateLoad(realelty, intcast);
2213+
}
2214+
else {
2215+
Compare = emit_unbox(ctx, realelty, cmp, jltype);
2216+
}
22012217
if (realelty != elty)
22022218
Compare = ctx.builder.CreateZExt(Compare, elty);
22032219
}
@@ -2244,28 +2260,36 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
22442260
if (realelty != elty)
22452261
realCompare = ctx.builder.CreateTrunc(realCompare, realelty);
22462262
if (intcast) {
2263+
assert(!isboxed);
22472264
ctx.builder.CreateStore(realCompare, ctx.builder.CreateBitCast(intcast, realCompare->getType()->getPointerTo()));
2248-
if (maybe_null_if_boxed)
2249-
realCompare = ctx.builder.CreateLoad(intcast->getAllocatedType(), intcast);
2265+
if (tracked_pointers)
2266+
realCompare = ctx.builder.CreateLoad(intcast_eltyp, intcast);
22502267
}
2251-
if (maybe_null_if_boxed) {
2252-
Value *first_ptr = isboxed ? Compare : extract_first_ptr(ctx, Compare);
2253-
if (first_ptr)
2254-
null_load_check(ctx, first_ptr, mod, var);
2268+
if (maybe_null_if_boxed && tracked_pointers) {
2269+
Value *first_ptr = isboxed ? realCompare : extract_first_ptr(ctx, realCompare);
2270+
assert(first_ptr);
2271+
null_load_check(ctx, first_ptr, mod, var);
22552272
}
2256-
if (intcast)
2273+
if (intcast && !tracked_pointers)
22572274
oldval = mark_julia_slot(intcast, jltype, NULL, ctx.tbaa().tbaa_stack);
22582275
else
22592276
oldval = mark_julia_type(ctx, realCompare, isboxed, jltype);
22602277
rhs = newval(oldval);
22612278
if (isboxed) {
22622279
r = boxed(ctx, rhs);
22632280
}
2264-
else if (Order != AtomicOrdering::NotAtomic || CountTrackedPointers(realelty).count) {
2281+
else if (intcast) {
2282+
emit_unbox_store(ctx, rhs, intcast, ctx.tbaa().tbaa_stack, intcast->getAlign());
2283+
r = ctx.builder.CreateLoad(realelty, intcast);
2284+
if (!tracked_pointers) // oldval is a slot, so put the oldval back
2285+
ctx.builder.CreateStore(realCompare, intcast);
2286+
}
2287+
else if (Order != AtomicOrdering::NotAtomic) {
2288+
assert(!tracked_pointers);
22652289
r = emit_unbox(ctx, realelty, rhs, jltype);
2266-
if (realelty != elty)
2267-
r = ctx.builder.CreateZExt(r, elty);
22682290
}
2291+
if (realelty != elty)
2292+
r = ctx.builder.CreateZExt(r, elty);
22692293
if (needlock)
22702294
emit_lockstate_value(ctx, needlock, true);
22712295
cmp = oldval;
@@ -2331,9 +2355,10 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
23312355
realinstr = ctx.builder.CreateTrunc(realinstr, realelty);
23322356
if (intcast) {
23332357
ctx.builder.CreateStore(realinstr, ctx.builder.CreateBitCast(intcast, realinstr->getType()->getPointerTo()));
2358+
// n.b. this oldval is only used for emit_f_is in this branch, so we know a priori that it does not need a gc-root
23342359
oldval = mark_julia_slot(intcast, jltype, NULL, ctx.tbaa().tbaa_stack);
23352360
if (maybe_null_if_boxed)
2336-
realinstr = ctx.builder.CreateLoad(intcast->getAllocatedType(), intcast);
2361+
realinstr = ctx.builder.CreateLoad(intcast_eltyp, intcast);
23372362
}
23382363
else {
23392364
oldval = mark_julia_type(ctx, realinstr, isboxed, jltype);
@@ -2373,20 +2398,23 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
23732398
ctx.builder.SetInsertPoint(DoneBB);
23742399
if (needlock)
23752400
emit_lockstate_value(ctx, needlock, false);
2376-
if (parent != NULL) {
2401+
if (parent != NULL && r && tracked_pointers && (!isboxed || !type_is_permalloc(rhs.typ))) {
23772402
if (isreplacefield || issetfieldonce) {
2378-
// TODO: avoid this branch if we aren't making a write barrier
23792403
BasicBlock *BB = BasicBlock::Create(ctx.builder.getContext(), "xchg_wb", ctx.f);
23802404
DoneBB = BasicBlock::Create(ctx.builder.getContext(), "done_xchg_wb", ctx.f);
23812405
ctx.builder.CreateCondBr(Success, BB, DoneBB);
23822406
ctx.builder.SetInsertPoint(BB);
23832407
}
2384-
if (r) {
2385-
if (!isboxed)
2386-
emit_write_multibarrier(ctx, parent, r, rhs.typ);
2387-
else if (!type_is_permalloc(rhs.typ))
2388-
emit_write_barrier(ctx, parent, r);
2408+
if (realelty != elty)
2409+
r = ctx.builder.Insert(CastInst::Create(Instruction::Trunc, r, realelty));
2410+
if (intcast) {
2411+
ctx.builder.CreateStore(r, intcast);
2412+
r = ctx.builder.CreateLoad(intcast_eltyp, intcast);
23892413
}
2414+
if (!isboxed)
2415+
emit_write_multibarrier(ctx, parent, r, rhs.typ);
2416+
else if (!type_is_permalloc(rhs.typ))
2417+
emit_write_barrier(ctx, parent, r);
23902418
if (isreplacefield || issetfieldonce) {
23912419
ctx.builder.CreateBr(DoneBB);
23922420
ctx.builder.SetInsertPoint(DoneBB);
@@ -2405,21 +2433,18 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
24052433
instr = ctx.builder.Insert(CastInst::Create(Instruction::Trunc, instr, realelty));
24062434
if (intcast) {
24072435
ctx.builder.CreateStore(instr, ctx.builder.CreateBitCast(intcast, instr->getType()->getPointerTo()));
2408-
instr = nullptr;
2436+
if (tracked_pointers)
2437+
instr = ctx.builder.CreateLoad(intcast_eltyp, intcast);
24092438
}
2410-
if (maybe_null_if_boxed) {
2411-
if (intcast)
2412-
instr = ctx.builder.CreateLoad(intcast->getAllocatedType(), intcast);
2439+
if (maybe_null_if_boxed && tracked_pointers) {
24132440
Value *first_ptr = isboxed ? instr : extract_first_ptr(ctx, instr);
2414-
if (first_ptr)
2415-
null_load_check(ctx, first_ptr, mod, var);
2416-
if (intcast && !first_ptr)
2417-
instr = nullptr;
2441+
assert(first_ptr);
2442+
null_load_check(ctx, first_ptr, mod, var);
24182443
}
2419-
if (instr)
2420-
oldval = mark_julia_type(ctx, instr, isboxed, jltype);
2421-
else
2444+
if (intcast && !tracked_pointers)
24222445
oldval = mark_julia_slot(intcast, jltype, NULL, ctx.tbaa().tbaa_stack);
2446+
else
2447+
oldval = mark_julia_type(ctx, instr, isboxed, jltype);
24232448
if (isreplacefield) {
24242449
Success = ctx.builder.CreateZExt(Success, getInt8Ty(ctx.builder.getContext()));
24252450
const jl_cgval_t argv[2] = {oldval, mark_julia_type(ctx, Success, false, jl_bool_type)};

‎test/atomics.jl‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ test_field_operators(ARefxy{Any}(123_10, 123_20))
128128
test_field_operators(ARefxy{Union{Nothing,Int}}(123_10, nothing))
129129
test_field_operators(ARefxy{Complex{Int32}}(123_10, 123_20))
130130
test_field_operators(ARefxy{Complex{Int128}}(123_10, 123_20))
131+
test_field_operators(ARefxy{Complex{Real}}(123_10, 123_20))
131132
test_field_operators(ARefxy{PadIntA}(123_10, 123_20))
132133
test_field_operators(ARefxy{PadIntB}(123_10, 123_20))
133134
#FIXME: test_field_operators(ARefxy{Int24}(123_10, 123_20))
@@ -316,6 +317,7 @@ test_field_orderings(ARefxy{Any}(true, false), true, false)
316317
test_field_orderings(ARefxy{Union{Nothing,Missing}}(nothing, missing), nothing, missing)
317318
test_field_orderings(ARefxy{Union{Nothing,Int}}(nothing, 123_1), nothing, 123_1)
318319
test_field_orderings(Complex{Int128}(10, 30), Complex{Int128}(20, 40))
320+
test_field_orderings(Complex{Real}(10, 30), Complex{Real}(20, 40))
319321
test_field_orderings(10.0, 20.0)
320322
test_field_orderings(NaN, Inf)
321323

@@ -481,6 +483,7 @@ test_global_operators(Any)
481483
test_global_operators(Union{Nothing,Int})
482484
test_global_operators(Complex{Int32})
483485
test_global_operators(Complex{Int128})
486+
test_global_operators(Complex{Real})
484487
test_global_operators(PadIntA)
485488
test_global_operators(PadIntB)
486489
#FIXME: test_global_operators(Int24)
@@ -604,6 +607,7 @@ test_global_orderings(Any, true, false)
604607
test_global_orderings(Union{Nothing,Missing}, nothing, missing)
605608
test_global_orderings(Union{Nothing,Int}, nothing, 123_1)
606609
test_global_orderings(Complex{Int128}, Complex{Int128}(10, 30), Complex{Int128}(20, 40))
610+
test_global_orderings(Complex{Real}, Complex{Real}(10, 30), Complex{Real}(20, 40))
607611
test_global_orderings(Float64, 10.0, 20.0)
608612
test_global_orderings(Float64, NaN, Inf)
609613

@@ -757,6 +761,7 @@ test_memory_operators(Any)
757761
test_memory_operators(Union{Nothing,Int})
758762
test_memory_operators(Complex{Int32})
759763
test_memory_operators(Complex{Int128})
764+
test_memory_operators(Complex{Real})
760765
test_memory_operators(PadIntA)
761766
test_memory_operators(PadIntB)
762767
#FIXME: test_memory_operators(Int24)
@@ -944,6 +949,7 @@ test_memory_orderings(Any, true, false)
944949
test_memory_orderings(Union{Nothing,Missing}, nothing, missing)
945950
test_memory_orderings(Union{Nothing,Int}, nothing, 123_1)
946951
test_memory_orderings(Complex{Int128}(10, 30), Complex{Int128}(20, 40))
952+
test_memory_orderings(Complex{Real}(10, 30), Complex{Real}(20, 40))
947953
test_memory_orderings(10.0, 20.0)
948954
test_memory_orderings(NaN, Inf)
949955

0 commit comments

Comments
 (0)