Skip to content

Commit 878ff5e

Browse files
vtjnashKristofferC
authored andcommitted
codegen: mark write barrier field load as volatile (#59559)
This is as an alternative to changing all allocation functions to mutating all memory and returning an aliasing pointer. This operates usually late in the pipeline, so either should not make much difference, but I think it should suffice to mark this volatile to prevent any de-duplication of this load, and this should also be most conservative for GC but more liberal for other optimizations. Fixes #59547 Produced with dubious help by Claude. (cherry picked from commit 218f691)
1 parent c124bf9 commit 878ff5e

File tree

3 files changed

+47
-3
lines changed

3 files changed

+47
-3
lines changed

Compiler/test/codegen.jl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,14 +133,14 @@ if !is_debug_build && opt_level > 0
133133
# Array
134134
test_loads_no_call(strip_debug_calls(get_llvm(sizeof, Tuple{Vector{Int}})), [Iptr])
135135
# As long as the eltype is known we don't need to load the elsize, but do need to check isvector
136-
@test_skip test_loads_no_call(strip_debug_calls(get_llvm(sizeof, Tuple{Array{Any}})), ["atomic $Iptr", "ptr", "ptr", Iptr, Iptr, "ptr", Iptr])
136+
@test_skip test_loads_no_call(strip_debug_calls(get_llvm(sizeof, Tuple{Array{Any}})), ["atomic volatile $Iptr", "ptr", "ptr", Iptr, Iptr, "ptr", Iptr])
137137
# Memory
138138
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory{Int}})), [Iptr])
139139
# As long as the eltype is known we don't need to load the elsize
140140
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory{Any}})), [Iptr])
141141
# Check that we load the elsize and isunion from the typeof layout
142-
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory})), [Iptr, "atomic $Iptr", "ptr", "i32", "i16"])
143-
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory})), [Iptr, "atomic $Iptr", "ptr", "i32", "i16"])
142+
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory})), [Iptr, "atomic volatile $Iptr", "ptr", "i32", "i16"])
143+
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Memory})), [Iptr, "atomic volatile $Iptr", "ptr", "i32", "i16"])
144144
# Primitive Type size should be folded to a constant
145145
test_loads_no_call(strip_debug_calls(get_llvm(core_sizeof, Tuple{Ptr})), String[])
146146

src/llvm-late-gc-lowering.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1899,6 +1899,9 @@ Value *LateLowerGCFrame::EmitLoadTag(IRBuilder<> &builder, Type *T_size, Value *
18991899
auto &M = *builder.GetInsertBlock()->getModule();
19001900
LoadInst *load = builder.CreateAlignedLoad(T_size, addr, M.getDataLayout().getPointerABIAlignment(0), V->getName() + ".tag");
19011901
load->setOrdering(AtomicOrdering::Unordered);
1902+
// Mark as volatile to prevent optimizers from treating GC tag loads as constants
1903+
// since GC mark bits can change during runtime (issue #59547)
1904+
load->setVolatile(true);
19021905
load->setMetadata(LLVMContext::MD_tbaa, tbaa_tag);
19031906
MDBuilder MDB(load->getContext());
19041907
auto *NullInt = ConstantInt::get(T_size, 0);
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
; This file is a part of Julia. License is MIT: https://julialang.org/license
2+
3+
; RUN: opt --load-pass-plugin=libjulia-codegen%shlibext -passes='function(LateLowerGCFrame,FinalLowerGC,gvn)' -S %s | FileCheck %s
4+
5+
; Test for issue #59547: Ensure write barrier GC tag loads are volatile
6+
; This test verifies that the LateLowerGCFrame pass marks GC tag loads as volatile
7+
; to prevent GVN from incorrectly constant-folding them, which would eliminate
8+
; necessary write barrier checks.
9+
10+
@tag = external addrspace(10) global {}, align 16
11+
12+
declare void @julia.write_barrier({} addrspace(10)*, {} addrspace(10)*)
13+
declare {}*** @julia.get_pgcstack()
14+
declare {} addrspace(10)* @julia.gc_alloc_obj({}**, i64, {} addrspace(10)*)
15+
16+
; Test that write barrier expansion produces volatile GC tag loads
17+
; CHECK-LABEL: @test_writebarrier_volatile_tags
18+
define {} addrspace(10)* @test_writebarrier_volatile_tags() {
19+
top:
20+
%pgcstack = call {}*** @julia.get_pgcstack()
21+
%current_task = bitcast {}*** %pgcstack to {}**
22+
%parent = call {} addrspace(10)* @julia.gc_alloc_obj({}** %current_task, i64 8, {} addrspace(10)* @tag)
23+
%child = call {} addrspace(10)* @julia.gc_alloc_obj({}** %current_task, i64 8, {} addrspace(10)* @tag)
24+
call void @julia.write_barrier({} addrspace(10)* %parent, {} addrspace(10)* %child)
25+
ret {} addrspace(10)* %parent
26+
27+
; The critical test: GC tag loads must be volatile to prevent constant folding
28+
; CHECK: load atomic volatile i64, ptr {{.*}} unordered, align 8, {{.*}}!tbaa
29+
; CHECK: and i64 {{.*}}, 3
30+
; CHECK: icmp eq i64 {{.*}}, 3
31+
; CHECK: br i1 {{.*}}, label %may_trigger_wb, label
32+
33+
; CHECK: may_trigger_wb:
34+
; CHECK: load atomic volatile i64, ptr {{.*}} unordered, align 8, {{.*}}!tbaa
35+
; CHECK: and i64 {{.*}}, 1
36+
; CHECK: icmp eq i64 {{.*}}, 0
37+
; CHECK: br i1 {{.*}}, label %trigger_wb, label
38+
39+
; CHECK: trigger_wb:
40+
; CHECK: call void @ijl_gc_queue_root(ptr {{.*}})
41+
}

0 commit comments

Comments
 (0)