From 88457ae249de9e7ab0be0ca7bec668e2180a66e7 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 24 Jul 2023 17:09:32 +0300 Subject: [PATCH] custom_insts: group all `debugPrintf`-like inputs of `Abort` together. --- .../src/builder/builder_methods.rs | 6 +-- .../src/builder/intrinsics.rs | 27 ++++++----- .../rustc_codegen_spirv/src/custom_insts.rs | 7 +-- .../src/linker/spirt_passes/controlflow.rs | 48 ++++++++++++------- 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index ef92f02da3..21ee0d8075 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -2878,11 +2878,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { // HACK(eddyb) redirect any possible panic call to an abort, to avoid // needing to materialize `&core::panic::Location` or `format_args!`. - self.abort_with_message_and_debug_printf_args( - // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. - format!("panicked|{message}"), - debug_printf_args, - ); + self.abort_with_kind_and_message_debug_printf("panic", message, debug_printf_args); self.undef(result_type) } else if let Some(mode) = buffer_load_intrinsic { self.codegen_buffer_load_intrinsic(result_type, args, mode) diff --git a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs index 982e47f13d..8089bd09b3 100644 --- a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs +++ b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs @@ -339,11 +339,7 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } fn abort(&mut self) { - // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. - self.abort_with_message_and_debug_printf_args( - "aborted|intrinsics::abort() called".into(), - [], - ); + self.abort_with_kind_and_message_debug_printf("abort", "intrinsics::abort() called", []); } fn assume(&mut self, _val: Self::Value) { @@ -378,24 +374,31 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } impl Builder<'_, '_> { - pub fn abort_with_message_and_debug_printf_args( + pub fn abort_with_kind_and_message_debug_printf( &mut self, - message: String, - debug_printf_args: impl IntoIterator, + kind: &str, + message_debug_printf_fmt_str: impl Into, + message_debug_printf_args: impl IntoIterator, ) { // FIXME(eddyb) this should be cached more efficiently. let void_ty = SpirvType::Void.def(rustc_span::DUMMY_SP, self); // HACK(eddyb) there is no `abort` or `trap` instruction in SPIR-V, // so the best thing we can do is use our own custom instruction. - let message_id = self.emit().string(message); + let kind_id = self.emit().string(kind); + let message_debug_printf_fmt_str_id = self.emit().string(message_debug_printf_fmt_str); self.custom_inst( void_ty, CustomInst::Abort { - message: Operand::IdRef(message_id), - debug_printf_args: debug_printf_args + kind: Operand::IdRef(kind_id), + message_debug_printf: [message_debug_printf_fmt_str_id] .into_iter() - .map(|arg| Operand::IdRef(arg.def(self))) + .chain( + message_debug_printf_args + .into_iter() + .map(|arg| arg.def(self)), + ) + .map(Operand::IdRef) .collect(), }, ); diff --git a/crates/rustc_codegen_spirv/src/custom_insts.rs b/crates/rustc_codegen_spirv/src/custom_insts.rs index 4ec02c5baa..8b7c9db1bf 100644 --- a/crates/rustc_codegen_spirv/src/custom_insts.rs +++ b/crates/rustc_codegen_spirv/src/custom_insts.rs @@ -151,9 +151,10 @@ def_custom_insts! { // users to do `catch_unwind` at the top-level of their shader to handle // panics specially (e.g. by appending to a custom buffer, or using some // specific color in a fragment shader, to indicate a panic happened). - // NOTE(eddyb) the `message` string follows `debugPrintf` rules, with remaining - // operands (collected into `debug_printf_args`) being its formatting arguments. - 4 => Abort { message, ..debug_printf_args }, + // NOTE(eddyb) `message_debug_printf` operands form a complete `debugPrintf` + // invocation (format string followed by inputs) for the "message", while + // `kind` only distinguishes broad categories like `"abort"` vs `"panic"`. + 4 => Abort { kind, ..message_debug_printf }, } impl CustomOp { diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs index e817fd4e2d..523896acdb 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs @@ -244,8 +244,8 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( if let Some(( func_at_abort_inst, CustomInst::Abort { - message, - debug_printf_args: message_debug_printf_args, + kind: abort_kind, + message_debug_printf, }, )) = custom_terminator_inst { @@ -335,26 +335,38 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( let mut fmt = String::new(); + let (message_debug_printf_fmt_str, message_debug_printf_args) = + message_debug_printf + .split_first() + .map(|(&fmt_str, args)| (&cx[const_str(fmt_str)], args)) + .unwrap_or_default(); + + let fmt_dbg_src_loc = |(file, line, col)| { + format!("{file}:{line}:{col}").replace('%', "%%") + }; + // HACK(eddyb) this improves readability w/ very verbose Vulkan loggers. fmt += "\n"; - fmt += "["; + fmt += "[Rust "; - // NB: `message` has its own `message_debug_printf_args` - // it formats, and as such any escaping is already done. - let message = &cx[const_str(message)]; - let (message_kind, message) = - message.split_once('|').unwrap_or(("", message)); + // HACK(eddyb) turn "panic" into "panicked", while the + // general case looks like "abort" -> "aborted". + match &cx[const_str(abort_kind)] { + "panic" => fmt += "panicked", + verb => { + fmt += verb; + fmt += "en"; + } + }; - if let Some((file, line, col)) = current_debug_src_loc.take() { - fmt += &format!("Rust {message_kind} at {file}:{line}:{col}") - .replace('%', "%%"); - } else { - fmt += message_kind; + if let Some(loc) = current_debug_src_loc.take() { + fmt += " at "; + fmt += &fmt_dbg_src_loc(loc); } fmt += "]\n "; - fmt += &message.replace('\n', "\n "); + fmt += &message_debug_printf_fmt_str.replace('\n', "\n "); let mut innermost = true; let mut append_call = |callsite_debug_src_loc, callee: &str| { @@ -368,9 +380,9 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( fmt += "\n called by "; } fmt += callee; - if let Some((file, line, col)) = callsite_debug_src_loc { - fmt += &format!("\n called at {file}:{line}:{col}") - .replace('%', "%%"); + if let Some(loc) = callsite_debug_src_loc { + fmt += "\n called at "; + fmt += &fmt_dbg_src_loc(loc); } current_debug_src_loc = callsite_debug_src_loc; }; @@ -391,7 +403,7 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( }); abort_inst_def.inputs = [Value::Const(mk_const_str(cx.intern(fmt)))] .into_iter() - .chain(message_debug_printf_args) + .chain(message_debug_printf_args.iter().copied()) .chain(debug_printf_context_inputs.iter().copied()) .collect();