Review feedback: add back sequence points and enforce tags only on sequence points or calls.

Author: cfallin

cranelift/codegen/meta/src/shared/instructions.rs

@@ -3902,4 +3902,20 @@ pub(crate) fn define(
             Operand::new("a", &TxN.dynamic_to_vector()).with_doc("New fixed vector"),
         ]),
     );
+
+    ig.push(
+        Inst::new(
+            "sequence_point",
+            r#"
+         A compiler barrier that acts as an immovable marker from IR input to machine-code output.
+
+         This "sequence point" can have debug tags attached to it, and these tags will be
+         noted in the output `MachBuffer`.
+
+         It prevents motion of any other side-effects across this boundary.
+         "#,
+            &formats.nullary,
+        )
+        .other_side_effects(),
+    );
 }

cranelift/codegen/src/cursor.rs

@@ -2,11 +2,7 @@
 //!
 //! This module defines cursor data types that can be used for inserting instructions.
 
-use crate::{
-    inst_predicates::has_lowering_side_effect,
-    ir::{self},
-};
-use alloc::vec::Vec;
+use crate::ir;
 
 /// The possible positions of a cursor.
 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
@@ -38,9 +34,6 @@ pub trait Cursor {
     /// Set the source location that should be assigned to new instructions.
     fn set_srcloc(&mut self, srcloc: ir::SourceLoc);
 
-    /// Set the debug tags that should be assigned to new side-effecting instructions.
-    fn set_debug_tags(&mut self, tags: Vec<ir::DebugTag>);
-
     /// Borrow a reference to the function layout that this cursor is navigating.
     fn layout(&self) -> &ir::Layout;
 
@@ -68,30 +61,6 @@ pub trait Cursor {
         self
     }
 
-    /// Exchange this cursor for one with a set debug tag list.
-    ///
-    /// These tags will be attached to all newly inserted
-    /// side-effecting instructions.
-    ///
-    /// This is intended to be used as a builder method:
-    ///
-    /// ```
-    /// # use cranelift_codegen::ir::{Function, Block, DebugTag};
-    /// # use cranelift_codegen::cursor::{Cursor, FuncCursor};
-    /// fn edit_func(func: &mut Function, tags: Vec<DebugTag>) {
-    ///     let mut pos = FuncCursor::new(func).with_debug_tags(tags);
-    ///
-    ///     // Use `pos`...
-    /// }
-    /// ```
-    fn with_debug_tags(mut self, tags: Vec<ir::DebugTag>) -> Self
-    where
-        Self: Sized,
-    {
-        self.set_debug_tags(tags);
-        self
-    }
-
     /// Rebuild this cursor positioned at `pos`.
     fn at_position(mut self, pos: CursorPosition) -> Self
     where
@@ -648,7 +617,6 @@ pub trait Cursor {
 pub struct FuncCursor<'f> {
     pos: CursorPosition,
     srcloc: ir::SourceLoc,
-    debug_tags: Vec<ir::DebugTag>,
 
     /// The referenced function.
     pub func: &'f mut ir::Function,
@@ -660,7 +628,6 @@ impl<'f> FuncCursor<'f> {
         Self {
             pos: CursorPosition::Nowhere,
             srcloc: Default::default(),
-            debug_tags: vec![],
             func,
         }
     }
@@ -694,10 +661,6 @@ impl<'f> Cursor for FuncCursor<'f> {
         self.srcloc = srcloc;
     }
 
-    fn set_debug_tags(&mut self, tags: Vec<ir::DebugTag>) {
-        self.debug_tags = tags;
-    }
-
     fn layout(&self) -> &ir::Layout {
         &self.func.layout
     }
@@ -721,9 +684,6 @@ impl<'c, 'f> ir::InstInserterBase<'c> for &'c mut FuncCursor<'f> {
         if !self.srcloc.is_default() {
             self.func.set_srcloc(inst, self.srcloc);
         }
-        if has_lowering_side_effect(self.func, inst) && !self.debug_tags.is_empty() {
-            self.func.debug_tags.set(inst, self.debug_tags.clone());
-        }
         &mut self.func.dfg
     }
 }

cranelift/codegen/src/inline.rs

@@ -367,8 +367,11 @@ fn inline_one(
     let mut last_inlined_block = inline_block_layout(func, call_block, callee, &entity_map);
 
     // Get a copy of debug tags on the call instruction; these are
-    // prepended to debug tags on inlined instructions.
+    // prepended to debug tags on inlined instructions. Remove them
+    // from the call itself as it will be rewritten to a jump (which
+    // cannot have tags).
     let call_debug_tags = func.debug_tags.get(call_inst).to_vec();
+    func.debug_tags.set(call_inst, []);
 
     // Translate each instruction from the callee into the caller,
     // appending them to their associated block in the caller.

cranelift/codegen/src/inst_predicates.rs

@@ -147,7 +147,8 @@ pub fn has_memory_fence_semantics(op: Opcode) -> bool {
         | Opcode::AtomicLoad
         | Opcode::AtomicStore
         | Opcode::Fence
-        | Opcode::Debugtrap => true,
+        | Opcode::Debugtrap
+        | Opcode::SequencePoint => true,
         Opcode::Call | Opcode::CallIndirect | Opcode::TryCall | Opcode::TryCallIndirect => true,
         op if op.can_trap() => true,
         _ => false,

cranelift/codegen/src/ir/debug_tags.rs

@@ -71,6 +71,11 @@ pub enum DebugTag {
 
 impl DebugTags {
     /// Set the tags on an instruction, overwriting existing tag list.
+    ///
+    /// Tags can only be set on call instructions (those for which
+    /// [`crate::Opcode::is_call()`] returns `true`) and on
+    /// `sequence_point` instructions. This property is checked by the
+    /// CLIF verifier.
     pub fn set(&mut self, inst: Inst, tags: impl IntoIterator<Item = DebugTag>) {
         let start = u32::try_from(self.tags.len()).unwrap();
         self.tags.extend(tags);
@@ -93,13 +98,22 @@ impl DebugTags {
         }
     }
 
+    /// Does the given instruction have any tags?
+    pub fn has(&self, inst: Inst) -> bool {
+        // We rely on the invariant that an entry in the map is
+        // present only if the list range is non-empty.
+        self.insts.contains_key(&inst)
+    }
+
     /// Clone the tags from one instruction to another.
     ///
     /// This clone is cheap (references the same underlying storage)
     /// because the tag lists are immutable.
     pub fn clone_tags(&mut self, from: Inst, to: Inst) {
         if let Some(range) = self.insts.get(&from).cloned() {
             self.insts.insert(to, range);
+        } else {
+            self.insts.remove(&to);
         }
     }
 

cranelift/codegen/src/ir/function.rs

@@ -191,13 +191,20 @@ pub struct FunctionStencil {
     /// interpreted by Cranelift, only preserved.
     pub srclocs: SourceLocs,
 
-    /// Opaque debug-info tags on instructions.
+    /// Opaque debug-info tags on sequence-point and call
+    /// instructions.
     ///
     /// These tags are not interpreted by Cranelift, and are passed
     /// through to compilation-result metadata. The only semantic
     /// structure that Cranelift imposes is that when inlining, it
     /// prepends the callsite call instruction's tags to the tags on
     /// inlined instructions.
+    ///
+    /// In order to ensure clarity around guaranteed compiler
+    /// behavior, tags are only permitted on instructions whose
+    /// presence and sequence will remain the same in the compiled
+    /// output: namely, `sequence_point` instructions and ordinary
+    /// call instructions.
     pub debug_tags: DebugTags,
 
     /// An optional global value which represents an expression evaluating to

cranelift/codegen/src/ir/layout.rs

@@ -756,7 +756,7 @@ mod tests {
     use super::*;
     use crate::cursor::{Cursor, CursorPosition};
     use crate::entity::EntityRef;
-    use crate::ir::{Block, DebugTag, Inst, SourceLoc};
+    use crate::ir::{Block, Inst, SourceLoc};
     use alloc::vec::Vec;
     use core::cmp::Ordering;
 
@@ -795,10 +795,6 @@ mod tests {
             unimplemented!()
         }
 
-        fn set_debug_tags(&mut self, _tags: Vec<DebugTag>) {
-            unimplemented!()
-        }
-
         fn layout(&self) -> &Layout {
             self.layout
         }

cranelift/codegen/src/isa/aarch64/inst.isle

@@ -997,6 +997,9 @@
        (LabelAddress (dst WritableReg)
                      (label MachLabel))
 
+       ;; A pseudoinstruction that serves as a sequence point.
+       (SequencePoint)
+
        ;; Emits an inline stack probe loop.
        ;;
        ;; Note that this is emitted post-regalloc so `start` and `end` can be
@@ -5203,3 +5206,8 @@
       (let ((dst WritableReg (temp_writable_reg $I64))
             (_ Unit (emit (MInst.LabelAddress dst label))))
         dst))
+
+;; Helper for creating a `SequencePoint` instruction.
+(decl a64_sequence_point () SideEffectNoResult)
+(rule (a64_sequence_point)
+      (SideEffectNoResult.Inst (MInst.SequencePoint)))

cranelift/codegen/src/isa/aarch64/inst/emit.rs

@@ -3547,6 +3547,10 @@ impl MachInstEmit for Inst {
                 sink.use_label_at_offset(offset, label, LabelUse::Adr21);
             }
 
+            &Inst::SequencePoint { .. } => {
+                // Nothing.
+            }
+
             &Inst::StackProbeLoop { start, end, step } => {
                 assert!(emit_info.0.enable_probestack());
 

cranelift/codegen/src/isa/aarch64/inst/mod.rs

@@ -919,6 +919,7 @@ fn aarch64_get_operands(inst: &mut Inst, collector: &mut impl OperandVisitor) {
         Inst::LabelAddress { dst, .. } => {
             collector.reg_def(dst);
         }
+        Inst::SequencePoint { .. } => {}
         Inst::StackProbeLoop { start, end, .. } => {
             collector.reg_early_def(start);
             collector.reg_use(end);
@@ -2893,6 +2894,9 @@ impl Inst {
                 let dst = pretty_print_reg(dst.to_reg());
                 format!("label_address {dst}, {label:?}")
             }
+            &Inst::SequencePoint {} => {
+                format!("sequence_point")
+            }
             &Inst::StackProbeLoop { start, end, step } => {
                 let start = pretty_print_reg(start.to_reg());
                 let end = pretty_print_reg(end);