diff --git a/.github/workflows/mingw.yml b/.github/workflows/mingw.yml index 7f8d05a634b4d8..ce53e33aaea46c 100644 --- a/.github/workflows/mingw.yml +++ b/.github/workflows/mingw.yml @@ -70,6 +70,19 @@ jobs: with: ruby-version: '3.2' + - name: Remove Strawberry Perl pkg-config + working-directory: + # `pkg-config.bat` included in Strawberry Perl is written in + # Perl and doesn't work when another msys2 `perl` precede its + # own `perl`. + # + # ``` + # Can't find C:\Strawberry\perl\bin\pkg-config.bat on PATH, '.' not in PATH. + # ``` + run: | + Get-Command pkg-config.bat | % { ren $_.path ($_.path + "~") } + shell: pwsh + - name: Misc system & package info working-directory: run: | diff --git a/gc.c b/gc.c index 90f2b29bfaf3fe..3d0935ad1a4f10 100644 --- a/gc.c +++ b/gc.c @@ -1755,7 +1755,6 @@ rb_gc_pointer_to_heap_p(VALUE obj) #define LAST_OBJECT_ID() (object_id_counter * OBJ_ID_INCREMENT) static VALUE id2ref_value = 0; static st_table *id2ref_tbl = NULL; -static bool id2ref_tbl_built = false; #if SIZEOF_SIZE_T == SIZEOF_LONG_LONG static size_t object_id_counter = 1; @@ -1947,6 +1946,7 @@ build_id2ref_i(VALUE obj, void *data) case T_CLASS: case T_MODULE: if (RCLASS(obj)->object_id) { + RUBY_ASSERT(!rb_objspace_garbage_object_p(obj)); st_insert(id2ref_tbl, RCLASS(obj)->object_id, obj); } break; @@ -1955,6 +1955,7 @@ build_id2ref_i(VALUE obj, void *data) break; default: if (rb_shape_obj_has_id(obj)) { + RUBY_ASSERT(!rb_objspace_garbage_object_p(obj)); st_insert(id2ref_tbl, rb_obj_id(obj), obj); } break; @@ -1979,12 +1980,12 @@ object_id_to_ref(void *objspace_ptr, VALUE object_id) // build_id2ref_i will most certainly malloc, which could trigger GC and sweep // objects we just added to the table. - bool gc_disabled = RTEST(rb_gc_disable_no_rest()); + // By calling rb_gc_disable() we also save having to handle potentially garbage objects. + bool gc_disabled = RTEST(rb_gc_disable()); { rb_gc_impl_each_object(objspace, build_id2ref_i, (void *)id2ref_tbl); } if (!gc_disabled) rb_gc_enable(); - id2ref_tbl_built = true; } VALUE obj; @@ -2036,10 +2037,9 @@ obj_free_object_id(VALUE obj) RUBY_ASSERT(FIXNUM_P(obj_id) || RB_TYPE_P(obj_id, T_BIGNUM)); if (!st_delete(id2ref_tbl, (st_data_t *)&obj_id, NULL)) { - // If we're currently building the table then it's not a bug. // The the object is a T_IMEMO/fields, then it's possible the actual object // has been garbage collected already. - if (id2ref_tbl_built && !RB_TYPE_P(obj, T_IMEMO)) { + if (!RB_TYPE_P(obj, T_IMEMO)) { rb_bug("Object ID seen, but not in _id2ref table: object_id=%llu object=%s", NUM2ULL(obj_id), rb_obj_info(obj)); } } diff --git a/test/mkmf/test_pkg_config.rb b/test/mkmf/test_pkg_config.rb index d0a2dc130ab11a..adf5fa6e92b9f5 100644 --- a/test/mkmf/test_pkg_config.rb +++ b/test/mkmf/test_pkg_config.rb @@ -26,7 +26,7 @@ def setup Cflags: -I${includedir}/cflags-I --cflags-other EOF - @pkg_config_path, ENV["PKG_CONFIG_PATH"] = ENV["PKG_CONFIG_PATH"], mkintpath(@fixtures_dir) + @pkg_config_path, ENV["PKG_CONFIG_PATH"] = ENV["PKG_CONFIG_PATH"], @fixtures_dir end end diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index 9eca30c7875ac6..e040463bbf5868 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -148,6 +148,18 @@ def test }, call_threshold: 2 end + def test_send_on_heap_object_in_spilled_arg + # This leads to a register spill, so not using `assert_compiles` + assert_runs 'Hash', %q{ + def entry(a1, a2, a3, a4, a5, a6, a7, a8, a9) + a9.itself.class + end + + entry(1, 2, 3, 4, 5, 6, 7, 8, {}) # profile + entry(1, 2, 3, 4, 5, 6, 7, 8, {}) + }, call_threshold: 2 + end + def test_invokebuiltin omit 'Test fails at the moment due to not handling optional parameters' assert_compiles '["."]', %q{ diff --git a/tool/zjit_bisect.rb b/tool/zjit_bisect.rb index 472a60e66c5df9..a4280a4ec2b8ab 100755 --- a/tool/zjit_bisect.rb +++ b/tool/zjit_bisect.rb @@ -1,12 +1,26 @@ #!/usr/bin/env ruby require 'logger' require 'open3' +require 'optparse' +require 'shellwords' require 'tempfile' require 'timeout' -RUBY = ARGV[0] || raise("Usage: ruby jit_bisect.rb ") -OPTIONS = ARGV[1] || raise("Usage: ruby jit_bisect.rb ") -TIMEOUT_SEC = 5 +ARGS = {timeout: 5} +OptionParser.new do |opts| + opts.banner += " -- " + opts.on("--timeout=TIMEOUT_SEC", "Seconds until child process is killed") do |timeout| + ARGS[:timeout] = Integer(timeout) + end + opts.on("-h", "--help", "Prints this help") do + puts opts + exit + end +end.parse! + +RUBY = ARGV[0] || raise("Usage: ruby jit_bisect.rb -- ") +OPTIONS = ARGV[1..] +raise("Usage: ruby jit_bisect.rb -- ") if OPTIONS.empty? LOGGER = Logger.new($stdout) # From https://github.com/tekknolagi/omegastar @@ -58,6 +72,29 @@ def run_bisect(command, items) bisect_impl(command, [], items) end +def run_ruby *cmd + stdout_data = nil + stderr_data = nil + status = nil + Open3.popen3(*cmd) do |stdin, stdout, stderr, wait_thr| + pid = wait_thr.pid + begin + Timeout.timeout(ARGS[:timeout]) do + stdout_data = stdout.read + stderr_data = stderr.read + status = wait_thr.value + end + rescue Timeout::Error + Process.kill("KILL", pid) + stderr_data = "(killed due to timeout)" + # Wait for the process to be reaped + wait_thr.value + status = 1 + end + end + [stdout_data, stderr_data, status] +end + def run_with_jit_list(ruby, options, jit_list) # Make a new temporary file containing the JIT list Tempfile.create("jit_list") do |temp_file| @@ -65,33 +102,33 @@ def run_with_jit_list(ruby, options, jit_list) temp_file.flush temp_file.close # Run the JIT with the temporary file - Open3.capture3("#{ruby} --zjit-allowed-iseqs=#{temp_file.path} #{options}") + run_ruby ruby, "--zjit-allowed-iseqs=#{temp_file.path}", *options end end # Try running with no JIT list to get a stable baseline -_, stderr, status = run_with_jit_list(RUBY, OPTIONS, []) -if !status.success? +_, stderr, exitcode = run_with_jit_list(RUBY, OPTIONS, []) +if exitcode != 0 raise "Command failed with empty JIT list: #{stderr}" end # Collect the JIT list from the failing Ruby process jit_list = nil Tempfile.create "jit_list" do |temp_file| - Open3.capture3("#{RUBY} --zjit-log-compiled-iseqs=#{temp_file.path} #{OPTIONS}") + run_ruby RUBY, "--zjit-log-compiled-iseqs=#{temp_file.path}", *OPTIONS jit_list = File.readlines(temp_file.path).map(&:strip).reject(&:empty?) end LOGGER.info("Starting with JIT list of #{jit_list.length} items.") # Now narrow it down command = lambda do |items| - status = Timeout.timeout(TIMEOUT_SEC) do - _, _, status = run_with_jit_list(RUBY, OPTIONS, items) - status - end - status.success? + _, _, exitcode = run_with_jit_list(RUBY, OPTIONS, items) + exitcode == 0 end result = run_bisect(command, jit_list) File.open("jitlist.txt", "w") do |file| file.puts(result) end +puts "Run:" +command = [RUBY, "--zjit-allowed-iseqs=jitlist.txt", *OPTIONS].shelljoin +puts command puts "Reduced JIT list (available in jitlist.txt):" puts result diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index 1bed45cba5f6aa..3263392cf6d371 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -111,7 +111,7 @@ impl Opnd }) }, - _ => unreachable!("memory operand with non-register base") + _ => unreachable!("memory operand with non-register base: {base:?}") } } diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 3e6bbfa605463e..1d6901bac4a73f 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -1088,9 +1088,15 @@ fn gen_guard_type(jit: &mut JITState, asm: &mut Assembler, val: lir::Opnd, guard } else if let Some(expected_class) = guard_type.runtime_exact_ruby_class() { asm_comment!(asm, "guard exact class for non-immediate types"); - let side_exit = side_exit(jit, state, GuardType(guard_type))?; + // If val isn't in a register, load it to use it as the base of Opnd::mem later. + // TODO: Max thinks codegen should not care about the shapes of the operands except to create them. (Shopify/ruby#685) + let val = match val { + Opnd::Reg(_) | Opnd::VReg { .. } => val, + _ => asm.load(val), + }; // Check if it's a special constant + let side_exit = side_exit(jit, state, GuardType(guard_type))?; asm.test(val, (RUBY_IMMEDIATE_MASK as u64).into()); asm.jnz(side_exit.clone()); diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 55041945d4b505..f2f990afdeada2 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -843,6 +843,22 @@ impl<'a> FunctionPrinter<'a> { } } +/// Pretty printer for [`Function`]. +pub struct FunctionGraphvizPrinter<'a> { + fun: &'a Function, + ptr_map: PtrPrintMap, +} + +impl<'a> FunctionGraphvizPrinter<'a> { + pub fn new(fun: &'a Function) -> Self { + let mut ptr_map = PtrPrintMap::identity(); + if cfg!(test) { + ptr_map.map_ptrs = true; + } + Self { fun, ptr_map } + } +} + /// Union-Find (Disjoint-Set) is a data structure for managing disjoint sets that has an interface /// of two operations: /// @@ -2115,6 +2131,10 @@ impl Function { Some(DumpHIR::Debug) => println!("Optimized HIR:\n{:#?}", &self), None => {}, } + + if get_option!(dump_hir_graphviz) { + println!("{}", FunctionGraphvizPrinter::new(&self)); + } } @@ -2293,6 +2313,87 @@ impl<'a> std::fmt::Display for FunctionPrinter<'a> { } } +struct HtmlEncoder<'a, 'b> { + formatter: &'a mut std::fmt::Formatter<'b>, +} + +impl<'a, 'b> std::fmt::Write for HtmlEncoder<'a, 'b> { + fn write_str(&mut self, s: &str) -> std::fmt::Result { + for ch in s.chars() { + match ch { + '<' => self.formatter.write_str("<")?, + '>' => self.formatter.write_str(">")?, + '&' => self.formatter.write_str("&")?, + '"' => self.formatter.write_str(""")?, + '\'' => self.formatter.write_str("'")?, + _ => self.formatter.write_char(ch)?, + } + } + Ok(()) + } +} + +impl<'a> std::fmt::Display for FunctionGraphvizPrinter<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + macro_rules! write_encoded { + ($f:ident, $($arg:tt)*) => { + HtmlEncoder { formatter: $f }.write_fmt(format_args!($($arg)*)) + }; + } + use std::fmt::Write; + let fun = &self.fun; + let iseq_name = iseq_get_location(fun.iseq, 0); + write!(f, "digraph G {{ # ")?; + write_encoded!(f, "{iseq_name}")?; + write!(f, "\n")?; + writeln!(f, "node [shape=plaintext];")?; + writeln!(f, "mode=hier; overlap=false; splines=true;")?; + for block_id in fun.rpo() { + writeln!(f, r#" {block_id} [label=<"#)?; + write!(f, r#"")?; + for insn_id in &fun.blocks[block_id.0].insns { + let insn_id = fun.union_find.borrow().find_const(*insn_id); + let insn = fun.find(insn_id); + if matches!(insn, Insn::Snapshot {..}) { + continue; + } + write!(f, r#"")?; + } + writeln!(f, "
{block_id}("#)?; + if !fun.blocks[block_id.0].params.is_empty() { + let mut sep = ""; + for param in &fun.blocks[block_id.0].params { + write_encoded!(f, "{sep}{param}")?; + let insn_type = fun.type_of(*param); + if !insn_type.is_subtype(types::Empty) { + write_encoded!(f, ":{}", insn_type.print(&self.ptr_map))?; + } + sep = ", "; + } + } + let mut edges = vec![]; + writeln!(f, ") 
"#)?; + if insn.has_output() { + let insn_type = fun.type_of(insn_id); + if insn_type.is_subtype(types::Empty) { + write_encoded!(f, "{insn_id} = ")?; + } else { + write_encoded!(f, "{insn_id}:{} = ", insn_type.print(&self.ptr_map))?; + } + } + if let Insn::Jump(ref target) | Insn::IfTrue { ref target, .. } | Insn::IfFalse { ref target, .. } = insn { + edges.push((insn_id, target.target)); + } + write_encoded!(f, "{}", insn.print(&self.ptr_map))?; + writeln!(f, " 
>];")?; + for (src, dst) in edges { + writeln!(f, " {block_id}:{src} -> {dst}:params;")?; + } + } + writeln!(f, "}}") + } +} + #[derive(Debug, Clone, PartialEq)] pub struct FrameState { iseq: IseqPtr, @@ -5145,6 +5246,81 @@ mod tests { } } +#[cfg(test)] +mod graphviz_tests { + use super::*; + use expect_test::{expect, Expect}; + + #[track_caller] + fn assert_optimized_graphviz(method: &str, expected: Expect) { + let iseq = crate::cruby::with_rubyvm(|| get_method_iseq("self", method)); + unsafe { crate::cruby::rb_zjit_profile_disable(iseq) }; + let mut function = iseq_to_hir(iseq).unwrap(); + function.optimize(); + function.validate().unwrap(); + let actual = format!("{}", FunctionGraphvizPrinter::new(&function)); + expected.assert_eq(&actual); + } + + #[test] + fn test_guard_fixnum_or_fixnum() { + eval(r#" + def test(x, y) = x | y + + test(1, 2) + "#); + assert_optimized_graphviz("test", expect![[r#" + digraph G { # test@<compiled>:2 + node [shape=plaintext]; + mode=hier; overlap=false; splines=true; + bb0 [label=< + + + + + + +
bb0(v0:BasicObject, v1:BasicObject, v2:BasicObject) 
PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, 29) 
v8:Fixnum = GuardType v1, Fixnum 
v9:Fixnum = GuardType v2, Fixnum 
v10:Fixnum = FixnumOr v8, v9 
Return v10 
>]; + } + "#]]); + } + + #[test] + fn test_multiple_blocks() { + eval(r#" + def test(c) + if c + 3 + else + 4 + end + end + + test(1) + test("x") + "#); + assert_optimized_graphviz("test", expect![[r#" + digraph G { # test@<compiled>:3 + node [shape=plaintext]; + mode=hier; overlap=false; splines=true; + bb0 [label=< + + + + + +
bb0(v0:BasicObject, v1:BasicObject) 
v3:CBool = Test v1 
IfFalse v3, bb1(v0, v1) 
v5:Fixnum[3] = Const Value(3) 
Return v5 
>]; + bb0:v4 -> bb1:params; + bb1 [label=< + + + +
bb1(v7:BasicObject, v8:BasicObject) 
v10:Fixnum[4] = Const Value(4) 
Return v10 
>]; + } + "#]]); + } +} + #[cfg(test)] mod opt_tests { use super::*; diff --git a/zjit/src/options.rs b/zjit/src/options.rs index 340812f089acf3..92f56b8916559f 100644 --- a/zjit/src/options.rs +++ b/zjit/src/options.rs @@ -37,6 +37,8 @@ pub struct Options { /// Dump High-level IR after optimization, right before codegen. pub dump_hir_opt: Option, + pub dump_hir_graphviz: bool, + /// Dump low-level IR pub dump_lir: bool, @@ -61,6 +63,7 @@ impl Default for Options { debug: false, dump_hir_init: None, dump_hir_opt: None, + dump_hir_graphviz: false, dump_lir: false, dump_disasm: false, perf: false, @@ -186,6 +189,7 @@ fn parse_option(str_ptr: *const std::os::raw::c_char) -> Option<()> { ("dump-hir" | "dump-hir-opt", "") => options.dump_hir_opt = Some(DumpHIR::WithoutSnapshot), ("dump-hir" | "dump-hir-opt", "all") => options.dump_hir_opt = Some(DumpHIR::All), ("dump-hir" | "dump-hir-opt", "debug") => options.dump_hir_opt = Some(DumpHIR::Debug), + ("dump-hir-graphviz", "") => options.dump_hir_graphviz = true, ("dump-hir-init", "") => options.dump_hir_init = Some(DumpHIR::WithoutSnapshot), ("dump-hir-init", "all") => options.dump_hir_init = Some(DumpHIR::All),