ZJIT: Add HIR Comment insn

Useful for adding comments to HIR/LIR/disasm that are not specific to an instruction.

Possibly most useful for temporarily printing more information
while debugging by keeping the statements interleaved with the HIR.

With a local change like this

    diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs
    index bff384f..de5b41caa1 100644
    --- a/zjit/src/hir.rs
    +++ b/zjit/src/hir.rs
    @@ -2491,6 +2491,8 @@ fn type_specialize(&mut self) {
                                cme = unsafe { rb_aliased_callable_method_entry(cme) };
                                def_type = unsafe { get_cme_def_type(cme) };
                            }
    +                       eprintln!("WTH");
    +                       hir_comment!(self, block, "HOWDY def_type {:?}", def_type);
                            if def_type == VM_METHOD_TYPE_ISEQ {
                                // TODO(max): Allow non-iseq; cache cme
                                // Only specialize positional-positional calls

The print statement (1) shows up early (anywhere)
whereas the HIR comment (2) is interleaved with the rest of
the HIR so you can easily correlate debug info to the compilation of
whatever function you are observing:

    1.  WTH
        Optimized HIR:
        fn test@test.rb:2:
        bb0():
          EntryPoint interpreter
          v1:BasicObject = LoadSelf
          Jump bb2(v1)
        bb1(v4:BasicObject):
          EntryPoint JIT(0)
          Jump bb2(v4)
        bb2(v6:BasicObject):
          v11:Fixnum[1] = Const Value(1)
    2.    # HOWDY def_type 0
          PatchPoint MethodRedefined(Object@0x10184ed10, foo@0x9c01, cme:0x101967060)

The comments show up in HIR, LIR, and disasm:

    bb2(v6:BasicObject):
      v11:Fixnum[1] = Const Value(1)
      # HOWDY def_type 0
      PatchPoint MethodRedefined(Object@0x1220fed10, foo@0x9c01, cme:0x123717060)

--

    PatchPoint Exit(PatchPoint(NoTracePoint))
    # Insn: v11 Const Value(1)
    # Insn: v18 # HOWDY def_type 0
    # Insn: v19 PatchPoint MethodRedefined(Object@0x103aaed00, foo@0x9c01, cme:0x103bc7080)
    PatchPoint Exit(PatchPoint(MethodRedefined(Object@0x103aaed00, foo@0x9c01, cme:0x103bc7080)))

--

    0x1230bc0f0: nop
    # Insn: v11 Const Value(1)
    # Insn: v18 # HOWDY def_type 0
    # Insn: v19 PatchPoint MethodRedefined(Object@0x103c6ed20, foo@0x9c01, cme:0x103d87070)
    0x1230bc0f4: nop

Author: rwstauner

zjit/src/codegen.rs

@@ -594,6 +594,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio
     }
 
     let out_opnd = match insn {
+        Insn::Comment { .. } => return Ok(()), // comment instruction, no code generation
         &Insn::Const { val: Const::Value(val) } => gen_const_value(val),
         &Insn::Const { val: Const::CPtr(val) } => gen_const_cptr(val),
         &Insn::Const { val: Const::CInt64(val) } => gen_const_long(val),

zjit/src/hir.rs

@@ -24,6 +24,26 @@ use SendFallbackReason::*;
 pub(crate) mod tests;
 mod opt_tests;
 
+#[allow(unused_macros)]
+macro_rules! hir_comment {
+    ($func:expr, $block:expr, $($arg:tt)*) => {
+        // If a diagnostic dump is requested, enrich it with HIR comments. Otherwise, avoid
+        // allocating comment strings or adding comment instructions that nobody can observe.
+        let enable_comment = $crate::options::get_option_ref!(dump_hir_init).is_some() ||
+            $crate::options::get_option_ref!(dump_hir_opt).is_some() ||
+            $crate::options::get_option_ref!(dump_hir_graphviz).is_some() ||
+            $crate::options::get_option!(dump_hir_iongraph) ||
+            $crate::options::get_option_ref!(dump_lir).is_some() ||
+            $crate::options::get_option_ref!(dump_disasm).is_some();
+        if enable_comment {
+            $func.push_comment($block, format!($($arg)*));
+        }
+    };
+}
+
+#[allow(unused_imports)]
+pub(crate) use hir_comment;
+
 /// An index of an [`Insn`] in a [`Function`]. This is a popular
 /// type since this effectively acts as a pointer to an [`Insn`].
 /// See also: [`Function::find`].
@@ -829,6 +849,9 @@ impl From<ID> for FieldName {
 /// helps with editing.
 #[derive(Debug, Clone)]
 pub enum Insn {
+    /// Comment that can be inserted into HIR for diagnostics.
+    Comment { message: String },
+
     Const { val: Const },
     /// SSA block parameter. Also used for function parameters in the function's entry block.
     Param,
@@ -1192,7 +1215,8 @@ pub enum Insn {
 macro_rules! for_each_operand_impl {
     ($self:expr, $visit_one:ident, $visit_many:ident) => {
         match $self {
-            Insn::Const { .. }
+            Insn::Comment { .. }
+            | Insn::Const { .. }
             | Insn::Param
             | Insn::LoadArg { .. }
             | Insn::Entries { .. }
@@ -1501,7 +1525,8 @@ impl Insn {
     /// Not every instruction returns a value. Return true if the instruction does and false otherwise.
     pub fn has_output(&self) -> bool {
         match self {
-            Insn::Jump(_)
+            Insn::Comment { .. }
+            | Insn::Jump(_)
             | Insn::Entries { .. }
             | Insn::CondBranch { .. } | Insn::EntryPoint { .. } | Insn::Return { .. }
             | Insn::PatchPoint { .. } | Insn::SetIvar { .. } | Insn::SetClassVar { .. } | Insn::ArrayExtend { .. }
@@ -1561,6 +1586,7 @@ impl Insn {
     fn effects_of(&self) -> Effect {
         const allocates: Effect = Effect::read_write(abstract_heaps::PC.union(abstract_heaps::Allocator), abstract_heaps::Allocator);
         match &self {
+            Insn::Comment { .. } => effects::Empty,
             Insn::Const { .. } => effects::Empty,
             Insn::Param { .. } => effects::Empty,
             Insn::LoadArg { .. } => effects::Empty,
@@ -1745,6 +1771,12 @@ impl Insn {
     /// Note: These are restrictions on the `write` `EffectSet` only. Even instructions with
     /// `read: effects::Any` could potentially be omitted.
     fn is_elidable(&self) -> bool {
+        // Comments intentionally have no semantic effect, but they are diagnostics that should
+        // survive DCE so optimized HIR dumps retain the information callers inserted.
+        if matches!(self, Insn::Comment { .. }) {
+            return false;
+        }
+
         abstract_heaps::Allocator.includes(self.effects_of().write_bits())
     }
 }
@@ -1813,6 +1845,7 @@ static REGEXP_FLAGS: &[(u32, &str)] = &[
 impl<'a> std::fmt::Display for InsnPrinter<'a> {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         match &self.inner {
+            Insn::Comment { message } => write!(f, "# {message}"),
             Insn::Const { val } => { write!(f, "Const {}", val.print(self.ptr_map)) }
             Insn::Param => { write!(f, "Param") }
             Insn::LoadArg { idx, id, .. } => { write!(f, "LoadArg :{id}@{idx}") }
@@ -2695,6 +2728,10 @@ impl Function {
         id
     }
 
+    pub fn push_comment(&mut self, block: BlockId, message: String) -> InsnId {
+        self.push_insn(block, Insn::Comment { message })
+    }
+
     // Add an instruction to an SSA block
     fn push_insn_id(&mut self, block: BlockId, insn_id: InsnId) -> InsnId {
         self.blocks[block.0].insns.push(insn_id);
@@ -2887,6 +2924,7 @@ impl Function {
             Insn::Param => unimplemented!("params should not be present in block.insns"),
             Insn::LoadArg { val_type, .. } => *val_type,
             Insn::SetGlobal { .. } | Insn::Jump(_) | Insn::Entries { .. } | Insn::EntryPoint { .. }
+            | Insn::Comment { .. }
             | Insn::CondBranch { .. } | Insn::Return { .. } | Insn::Throw { .. }
             | Insn::PatchPoint { .. } | Insn::SetIvar { .. } | Insn::SetClassVar { .. } | Insn::ArrayExtend { .. }
             | Insn::ArrayPush { .. } | Insn::SideExit { .. } | Insn::SetLocal { .. }
@@ -5952,6 +5990,7 @@ impl Function {
         match insn {
             // Instructions with no InsnId operands (except state) or nothing to assert
             Insn::Const { .. }
+            | Insn::Comment { .. }
             | Insn::Param
             | Insn::LoadArg { .. }
             | Insn::PutSpecialObject { .. }

zjit/src/hir/opt_tests.rs

@@ -2413,6 +2413,24 @@ mod hir_opt_tests {
         ");
     }
 
+    #[test]
+    fn test_do_not_eliminate_comment() {
+        let mut function = Function::new(std::ptr::null());
+        let block = function.entry_block;
+
+        let comment = function.push_comment(block, "diagnostic".to_string());
+        let dead_const = function.push_insn(block, Insn::Const { val: Const::CBool(false) });
+        let return_val = function.push_insn(block, Insn::Const { val: Const::CBool(true) });
+        function.push_insn(block, Insn::Return { val: return_val });
+        function.seal_entries();
+
+        function.eliminate_dead_code();
+
+        let insns = &function.blocks[block.0].insns;
+        assert!(insns.contains(&comment));
+        assert!(!insns.contains(&dead_const));
+    }
+
     #[test]
     fn test_eliminate_new_array() {
         eval("