codegen: phic/upsilon nodes can be set to undefined (#37319)

Unlike other nodes, Upsilon nodes can be potentially undefined and need special handling to ensure they aren't literally undef, as we might try to union-copy or gc-root the data later. Fix #37262
JuliaLang · Sep 3, 2020 · 5cf5717 · 5cf5717
1 parent 5c3557e
commit 5cf5717
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 30 deletions.
diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -3961,7 +3961,7 @@ static void emit_ssaval_assign(jl_codectx_t &ctx, ssize_t idx, jl_value_t *r)
 
 static void emit_varinfo_assign(jl_codectx_t &ctx, jl_varinfo_t &vi, jl_cgval_t rval_info, jl_value_t *l=NULL)
 {
-    if (!vi.used)
+    if (!vi.used || vi.value.typ == jl_bottom_type)
         return;
 
     // convert rval-type to lval-type
@@ -4048,6 +4048,49 @@ static void emit_assignment(jl_codectx_t &ctx, jl_value_t *l, jl_value_t *r, ssi
     emit_varinfo_assign(ctx, vi, rval_info, l);
 }
 
+static void emit_upsilonnode(jl_codectx_t &ctx, ssize_t phic, jl_value_t *val)
+{
+    jl_varinfo_t &vi = ctx.phic_slots[phic];
+    // If the val is null, we can ignore the store.
+    // The middle end guarantees that the value from this
+    // upsilon node is not dynamically observed.
+    if (val) {
+        jl_cgval_t rval_info = emit_expr(ctx, val);
+        if (rval_info.typ == jl_bottom_type)
+            // as a special case, PhiC nodes are allowed to use undefined
+            // values, since they are just copy operations, so we need to
+            // ignore the store (it will not by dynamically observed), while
+            // normally, for any other operation result, we'd assume this store
+            // was unreachable and dead
+            val = NULL;
+        else
+            emit_varinfo_assign(ctx, vi, rval_info);
+    }
+    if (!val) {
+        if (vi.boxroot) {
+            // memory optimization: eagerly clear this gc-root now
+            ctx.builder.CreateAlignedStore(V_rnull, vi.boxroot, true, Align(sizeof(void*)));
+        }
+        if (vi.pTIndex) {
+            // We don't care what the contents of the variable are, but it
+            // does need to satisfy the union invariants (i.e. inbounds
+            // tindex).
+            ctx.builder.CreateAlignedStore(
+                vi.boxroot ? ConstantInt::get(T_int8, 0x80) :
+                             ConstantInt::get(T_int8, 0x01),
+                vi.pTIndex, true, Align(1));
+        }
+        else if (vi.value.V && !vi.value.constant && vi.value.typ != jl_bottom_type) {
+            assert(vi.value.ispointer());
+            Type *T = cast<AllocaInst>(vi.value.V)->getAllocatedType();
+            if (CountTrackedPointers(T).count) {
+                // make sure gc pointers (including ptr_phi of union-split) are initialized to NULL
+                ctx.builder.CreateStore(Constant::getNullValue(T), vi.value.V, true);
+            }
+        }
+    }
+}
+
 // --- convert expression to code ---
 
 static jl_cgval_t emit_cfunction(jl_codectx_t &ctx, jl_value_t *output_type, const jl_cgval_t &fexpr, jl_value_t *rt, jl_svec_t *argt);
@@ -6052,10 +6095,7 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
             i == 0) { // or it is the first argument (which isn't in `argArray`)
             AllocaInst *av = new AllocaInst(T_prjlvalue, 0,
                 jl_symbol_name(s), /*InsertBefore*/ctx.ptlsStates);
-            StoreInst *SI = new StoreInst(
-                ConstantPointerNull::get(cast<PointerType>(T_prjlvalue)), av,
-                false,
-                Align(sizeof(void*)));
+            StoreInst *SI = new StoreInst(V_rnull, av, false, Align(sizeof(void*)));
             SI->insertAfter(ctx.ptlsStates);
             varinfo.boxroot = av;
             if (ctx.debug_enabled && varinfo.dinfo) {
@@ -6657,23 +6697,7 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
             continue;
         }
         if (jl_is_upsilonnode(stmt)) {
-            jl_value_t *val = jl_fieldref_noalloc(stmt, 0);
-            // If the val is null, we can ignore the store.
-            // The middle end guarantees that the value from this
-            // upsilon node is not dynamically observed.
-            jl_varinfo_t &vi = ctx.phic_slots[upsilon_to_phic[cursor+1]];
-            if (val) {
-                jl_cgval_t rval_info = emit_expr(ctx, val);
-                emit_varinfo_assign(ctx, vi, rval_info);
-            } else if (vi.pTIndex) {
-                // We don't care what the contents of the variable are, but it
-                // does need to satisfy the union invariants (i.e. inbounds
-                // tindex).
-                ctx.builder.CreateStore(
-                    vi.boxroot ? ConstantInt::get(T_int8, 0x80) :
-                                 ConstantInt::get(T_int8, 0x01),
-                    vi.pTIndex, true);
-            }
+            emit_upsilonnode(ctx, upsilon_to_phic[cursor + 1], jl_fieldref_noalloc(stmt, 0));
             find_next_stmt(cursor + 1);
             continue;
         }

diff --git a/test/compiler/codegen.jl b/test/compiler/codegen.jl
@@ -10,14 +10,8 @@ const coverage = (Base.JLOptions().code_coverage > 0) || (Base.JLOptions().mallo
 const Iptr = sizeof(Int) == 8 ? "i64" : "i32"
 
 # `_dump_function` might be more efficient but it doesn't really matter here...
-get_llvm(@nospecialize(f), @nospecialize(t), strip_ir_metadata=true, dump_module=false) =
-    sprint(code_llvm, f, t, strip_ir_metadata, dump_module)
-
-get_llvm_noopt(@nospecialize(f), @nospecialize(t), strip_ir_metadata=true, dump_module=false) =
-    InteractiveUtils._dump_function(f, t,
-                #=native=# false, #=wrapper=# false, #=strip=# strip_ir_metadata,
-                #=dump_module=# dump_module, #=syntax=#:att, #=optimize=#false)
-
+get_llvm(@nospecialize(f), @nospecialize(t), raw=true, dump_module=false, optimize=true) =
+    sprint(code_llvm, f, t, raw, dump_module, optimize)
 
 if opt_level > 0
     # Make sure getptls call is removed at IR level with optimization on
@@ -456,3 +450,26 @@ end
     str_36422 = "using InteractiveUtils; code_llvm(Base.ht_keyindex, (Dict{NTuple{65,Int64},Nothing}, NTuple{65,Int64}))"
     @test success(`$(Base.julia_cmd()) --startup-file=no -e $str_36422`)
 end
+
+@noinline g37262(x) = (x ? error("intentional") : (0x1, "v", "1", ".", "2"))
+function f37262(x)
+    try
+        GC.safepoint()
+    catch
+        GC.safepoint()
+    end
+    try
+        GC.gc()
+        return g37262(x)
+    catch ex
+        GC.gc()
+    finally
+        GC.gc()
+    end
+end
+@testset "#37262" begin
+    str = "store volatile { i8, {}*, {}*, {}*, {}* } zeroinitializer, { i8, {}*, {}*, {}*, {}* }* %phic"
+    llvmstr = get_llvm(f37262, (Bool,), false, false, false)
+    @test contains(llvmstr, str) || llvmstr
+    @test f37262(Base.inferencebarrier(true)) === nothing
+end