Merge pull request #22912 from JuliaLang/kf/fixciregresssion

Fix recent CI compile time perf regression
JuliaLang · Jul 22, 2017 · 0b95a5d · 0b95a5d
2 parents 7181300 + 2d6a589
commit 0b95a5d
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 19 deletions.
diff --git a/base/options.jl b/base/options.jl
@@ -34,11 +34,18 @@ struct JLOptions
     bindto::Ptr{UInt8}
     outputbc::Ptr{UInt8}
     outputunoptbc::Ptr{UInt8}
+    outputjitbc::Ptr{UInt8}
     outputo::Ptr{UInt8}
     outputji::Ptr{UInt8}
     incremental::Int8
 end
 
+# This runs early in the sysimage != is not defined yet
+if sizeof(JLOptions) === ccall(:jl_sizeof_jl_options, Int, ())
+else
+    ccall(:jl_throw, Void, (Any,), "Option structure mismatch")
+end
+
 JLOptions() = unsafe_load(cglobal(:jl_options, JLOptions))
 
 function show(io::IO, opt::JLOptions)

diff --git a/doc/src/devdocs/llvm.md b/doc/src/devdocs/llvm.md
@@ -80,6 +80,12 @@ cc -shared -o sys.so sys.o
 ```
 This system image can then be loaded by `julia` as usual.
 
+Alternatively, you can
+use `--output-jit-bc jit.bc` to obtain a trace of all IR passed to the JIT.
+This is useful for code that cannot be run as part of the sysimg generation
+process (e.g. because it creates unserializable state). However, the resulting
+`jit.bc` does not include sysimage data, and can thus not be used as such.
+
 It is also possible to dump an LLVM IR module for just one Julia function,
 using:
 ```julia

diff --git a/src/jitlayers.cpp b/src/jitlayers.cpp
@@ -947,7 +947,7 @@ void* jl_get_globalvar(GlobalVariable *gv)
 void jl_add_to_shadow(Module *m)
 {
 #ifndef KEEP_BODIES
-    if (!imaging_mode)
+    if (!imaging_mode && !jl_options.outputjitbc)
         return;
 #endif
     ValueToValueMapTy VMap;
@@ -1064,7 +1064,6 @@ extern "C"
 void jl_dump_native(const char *bc_fname, const char *unopt_bc_fname, const char *obj_fname, const char *sysimg_data, size_t sysimg_len)
 {
     JL_TIMING(NATIVE_DUMP);
-    assert(imaging_mode);
     // We don't want to use MCJIT's target machine because
     // it uses the large code model and we may potentially
     // want less optimizations there.
@@ -1161,7 +1160,8 @@ void jl_dump_native(const char *bc_fname, const char *unopt_bc_fname, const char
 #endif
 
     // add metadata information
-    jl_gen_llvm_globaldata(shadow_output, sysimg_data, sysimg_len);
+    if (imaging_mode)
+        jl_gen_llvm_globaldata(shadow_output, sysimg_data, sysimg_len);
 
     // do the actual work
     PM.run(*shadow_output);

diff --git a/src/jloptions.c b/src/jloptions.c
@@ -127,7 +127,11 @@ static const char opts[]  =
     // compiler output options
     " --output-o name           Generate an object file (including system image data)\n"
     " --output-ji name          Generate a system image data file (.ji)\n"
-    " --output-unopt-bc name    Generate unoptimized LLVM bitcode (.bc)\n"
+// These are for compiler debugging purposes only and should not be otherwise
+// used, so don't show them here. See the devdocs for tips on using these
+// options for debugging the compiler.
+//  " --output-unopt-bc name    Generate unoptimized LLVM bitcode (.bc)\n"
+//  " --output-jit-bc name      Dump all IR generated by the frontend (not including system image)\n"
     " --output-bc name          Generate LLVM bitcode (.bc)\n"
     " --output-incremental=no   Generate an incremental output file (rather than complete)\n\n"
 
@@ -148,6 +152,7 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp)
            opt_code_coverage,
            opt_track_allocation,
            opt_check_bounds,
+           opt_output_jit_bc,
            opt_output_unopt_bc,
            opt_output_bc,
            opt_depwarn,
@@ -191,6 +196,7 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp)
         { "check-bounds",    required_argument, 0, opt_check_bounds },
         { "output-bc",       required_argument, 0, opt_output_bc },
         { "output-unopt-bc", required_argument, 0, opt_output_unopt_bc },
+        { "output-jit-bc",   required_argument, 0, opt_output_jit_bc },
         { "output-o",        required_argument, 0, opt_output_o },
         { "output-ji",       required_argument, 0, opt_output_ji },
         { "output-incremental",required_argument, 0, opt_incremental },
@@ -439,6 +445,9 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp)
             jl_options.outputbc = optarg;
             if (!jl_options.image_file_specified) jl_options.image_file = NULL;
             break;
+        case opt_output_jit_bc:
+            jl_options.outputjitbc = optarg;
+            break;
         case opt_output_unopt_bc:
             jl_options.outputunoptbc = optarg;
             if (!jl_options.image_file_specified) jl_options.image_file = NULL;
@@ -542,3 +551,8 @@ JL_DLLEXPORT void jl_set_ARGS(int argc, char **argv)
         }
     }
 }
+
+JL_DLLEXPORT ssize_t jl_sizeof_jl_options(void)
+{
+    return sizeof(jl_options_t);
+}
diff --git a/src/julia.h b/src/julia.h
@@ -1734,13 +1734,15 @@ typedef struct {
     const char *bindto;
     const char *outputbc;
     const char *outputunoptbc;
+    const char *outputjitbc;
     const char *outputo;
     const char *outputji;
     int8_t incremental;
     int8_t image_file_specified;
 } jl_options_t;
 
 extern JL_DLLEXPORT jl_options_t jl_options;
+JL_DLLEXPORT ssize_t jl_sizeof_jl_options(void);
 
 // Parse an argc/argv pair to extract general julia options, passing back out
 // any arguments that should be passed on to the script.

diff --git a/src/llvm-late-gc-lowering.cpp b/src/llvm-late-gc-lowering.cpp
@@ -1188,7 +1188,7 @@ static void AddInPredLiveOuts(BasicBlock *BB, BitVector &LiveIn, State &S)
         BB = &*WorkList.back();
         WorkList.pop_back();
         for (BasicBlock *Pred : predecessors(BB)) {
-            if (Visited.insert(Pred).second)
+            if (!Visited.insert(Pred).second)
                 continue;
             if (!S.BBStates[Pred].HasSafepoint) {
                 WorkList.push_back(Pred);
@@ -1206,7 +1206,9 @@ static void AddInPredLiveOuts(BasicBlock *BB, BitVector &LiveIn, State &S)
     }
 }
 
-void LateLowerGCFrame::PlaceGCFrameStore(State &S, unsigned R, unsigned MinColorRoot, const std::vector<int> &Colors, Value *GCFrame, Instruction *InsertionPoint) {
+void LateLowerGCFrame::PlaceGCFrameStore(State &S, unsigned R, unsigned MinColorRoot,
+                                         const std::vector<int> &Colors, Value *GCFrame,
+                                         Instruction *InsertionPoint) {
     Value *Val = GetPtrForNumber(S, R, InsertionPoint);
     Value *args[1] = {
         ConstantInt::get(T_int32, Colors[R]+MinColorRoot)
@@ -1222,26 +1224,27 @@ void LateLowerGCFrame::PlaceGCFrameStore(State &S, unsigned R, unsigned MinColor
     new StoreInst(Val, gep, InsertionPoint);
 }
 
-void LateLowerGCFrame::PlaceGCFrameStores(Function &F, State &S, unsigned MinColorRoot, const std::vector<int> &Colors, Value *GCFrame)
+void LateLowerGCFrame::PlaceGCFrameStores(Function &F, State &S, unsigned MinColorRoot,
+                                          const std::vector<int> &Colors, Value *GCFrame)
 {
     for (auto &BB : F) {
-        if (!S.BBStates[&BB].HasSafepoint) {
+        const BBState &BBS = S.BBStates[&BB];
+        if (!BBS.HasSafepoint) {
             continue;
         }
         BitVector LiveIn;
         AddInPredLiveOuts(&BB, LiveIn, S);
-        for(auto rit = S.BBStates[&BB].Safepoints.rbegin();
-              rit != S.BBStates[&BB].Safepoints.rend(); ++rit ) {
-            // Find those that become live, but were not before
-            BitVector NowLive = S.LiveSets[*rit];
-            LiveIn.resize(NowLive.size(), 0);
-            LiveIn.flip();
-            NowLive &= LiveIn;
+        const BitVector *LastLive = &LiveIn;
+        for(auto rit = BBS.Safepoints.rbegin();
+              rit != BBS.Safepoints.rend(); ++rit ) {
+            const BitVector &NowLive = S.LiveSets[*rit];
             for (int Idx = NowLive.find_first(); Idx >= 0; Idx = NowLive.find_next(Idx)) {
-                PlaceGCFrameStore(S, Idx, MinColorRoot, Colors, GCFrame,
-                  S.ReverseSafepointNumbering[*rit]);
+                if (!HasBitSet(*LastLive, Idx)) {
+                    PlaceGCFrameStore(S, Idx, MinColorRoot, Colors, GCFrame,
+                      S.ReverseSafepointNumbering[*rit]);
+                }
             }
-            LiveIn = S.LiveSets[*rit];
+            LastLive = &NowLive;
         }
     }
 }

diff --git a/src/precompile.c b/src/precompile.c
@@ -24,8 +24,11 @@ void jl_precompile(int all);
 
 void jl_write_compiler_output(void)
 {
-    if (!jl_generating_output())
+    if (!jl_generating_output()) {
+        if (jl_options.outputjitbc)
+            jl_dump_native(NULL, jl_options.outputjitbc, NULL, NULL, 0);
         return;
+    }
 
     if (!jl_options.incremental)
         jl_precompile(jl_options.compile_enabled == JL_OPTIONS_COMPILE_ALL);
@@ -35,6 +38,10 @@ void jl_write_compiler_output(void)
         return;
     }
 
+    if (jl_options.outputjitbc) {
+        jl_printf(JL_STDERR, "WARNING: --output-jit-bc is meaningless with options for dumping sysimage data\n");
+    }
+
     jl_array_t *worklist = jl_module_init_order;
     JL_GC_PUSH1(&worklist);
     jl_module_init_order = jl_alloc_vec_any(0);

diff --git a/test/llvmpasses/gcroots.ll b/test/llvmpasses/gcroots.ll
@@ -5,6 +5,7 @@
 declare void @boxed_simple(%jl_value_t addrspace(10)*, %jl_value_t addrspace(10)*)
 declare %jl_value_t addrspace(10)* @jl_box_int64(i64)
 declare %jl_value_t*** @jl_get_ptls_states()
+declare void @jl_safepoint()
 declare %jl_value_t addrspace(10)* @jl_apply_generic(%jl_value_t addrspace(10)*, %jl_value_t addrspace(10)**, i32)
 
 define void @simple(i64 %a, i64 %b) {
@@ -184,3 +185,25 @@ define void @global_ref() {
     call void @one_arg_boxed(%jl_value_t addrspace(10)* %loaded)
     ret void
 }
+
+define %jl_value_t addrspace(10)* @no_redundant_rerooting(i64 %a, i1 %cond) {
+; CHECK-LABEL: @no_redundant_rerooting
+; CHECK: %gcframe = alloca %jl_value_t addrspace(10)*, i32 3
+top:
+    %ptls = call %jl_value_t*** @jl_get_ptls_states()
+    %aboxed = call %jl_value_t addrspace(10)* @jl_box_int64(i64 signext %a)
+; CHECK: store %jl_value_t addrspace(10)* %aboxed 
+; CHECK-NEXT: call void @jl_safepoint()
+    call void @jl_safepoint()
+    br i1 %cond, label %blocka, label %blockb
+blocka:
+; CHECK-NOT: call void @jl_safepoint()
+; CHECK: call void @jl_safepoint()
+    call void @jl_safepoint()
+    ret %jl_value_t addrspace(10)* %aboxed
+blockb:
+; CHECK-NOT: call void @jl_safepoint()
+; CHECK: call void @jl_safepoint()
+    call void @jl_safepoint()
+    ret %jl_value_t addrspace(10)* %aboxed
+}