Merge pull request #3432 from bjorn3/remove_reloc_constant

ConstantData related cleanups for the removal of the old backend

cranelift/codegen/src/binemit/memorysink.rs

@@ -15,7 +15,7 @@
 //! `CodeSink::put*` methods, so the performance impact of the virtual callbacks is less severe.
 use super::{Addend, CodeInfo, CodeOffset, CodeSink, Reloc};
 use crate::binemit::stack_map::StackMap;
-use crate::ir::{ConstantOffset, ExternalName, Opcode, SourceLoc, TrapCode};
+use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode};
 use core::ptr::write_unaligned;
 
 /// A `CodeSink` that writes binary machine code directly into memory.
@@ -79,9 +79,6 @@ pub trait RelocSink {
         _: Addend,
     );
 
-    /// Add a relocation referencing a constant.
-    fn reloc_constant(&mut self, _: CodeOffset, _: Reloc, _: ConstantOffset);
-
     /// Track a call site whose return address is the given CodeOffset, for the given opcode. Does
     /// nothing in general, only useful for certain embedders (SpiderMonkey).
     fn add_call_site(&mut self, _: Opcode, _: CodeOffset, _: SourceLoc) {}
@@ -138,11 +135,6 @@ impl<'a> CodeSink for MemoryCodeSink<'a> {
         self.relocs.reloc_external(ofs, srcloc, rel, name, addend);
     }
 
-    fn reloc_constant(&mut self, rel: Reloc, constant_offset: ConstantOffset) {
-        let ofs = self.offset();
-        self.relocs.reloc_constant(ofs, rel, constant_offset);
-    }
-
     fn trap(&mut self, code: TrapCode, srcloc: SourceLoc) {
         let ofs = self.offset();
         self.traps.trap(ofs, srcloc, code);
@@ -186,7 +178,6 @@ impl RelocSink for NullRelocSink {
         _: Addend,
     ) {
     }
-    fn reloc_constant(&mut self, _: CodeOffset, _: Reloc, _: ConstantOffset) {}
 }
 
 /// A `TrapSink` implementation that does nothing, which is convenient when

cranelift/codegen/src/binemit/mod.rs

@@ -11,7 +11,7 @@ pub use self::memorysink::{
     TrapSink,
 };
 pub use self::stack_map::StackMap;
-use crate::ir::{ConstantOffset, ExternalName, Opcode, SourceLoc, TrapCode};
+use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode};
 use core::fmt;
 #[cfg(feature = "enable-serde")]
 use serde::{Deserialize, Serialize};
@@ -35,8 +35,6 @@ pub enum Reloc {
     Abs8,
     /// x86 PC-relative 4-byte
     X86PCRel4,
-    /// x86 PC-relative 4-byte offset to trailing rodata
-    X86PCRelRodata4,
     /// x86 call to PC-relative 4-byte
     X86CallPCRel4,
     /// x86 call to PLT-relative 4-byte
@@ -78,7 +76,6 @@ impl fmt::Display for Reloc {
             Self::Abs8 => write!(f, "Abs8"),
             Self::S390xPCRel32Dbl => write!(f, "PCRel32Dbl"),
             Self::X86PCRel4 => write!(f, "PCRel4"),
-            Self::X86PCRelRodata4 => write!(f, "PCRelRodata4"),
             Self::X86CallPCRel4 => write!(f, "CallPCRel4"),
             Self::X86CallPLTRel4 => write!(f, "CallPLTRel4"),
             Self::X86GOTPCRel4 => write!(f, "GOTPCRel4"),
@@ -147,9 +144,6 @@ pub trait CodeSink {
     /// Add a relocation referencing an external symbol plus the addend at the current offset.
     fn reloc_external(&mut self, _: SourceLoc, _: Reloc, _: &ExternalName, _: Addend);
 
-    /// Add a relocation referencing a constant.
-    fn reloc_constant(&mut self, _: Reloc, _: ConstantOffset);
-
     /// Add trap information for the current offset.
     fn trap(&mut self, _: TrapCode, _: SourceLoc);
 

cranelift/codegen/src/ir/constant.rs

@@ -167,38 +167,6 @@ impl FromStr for ConstantData {
     }
 }
 
-/// This type describes an offset in bytes within a constant pool.
-pub type ConstantOffset = u32;
-
-/// Inner type for storing data and offset together in the constant pool. The offset is optional
-/// because it must be set relative to the function code size (i.e. constants are emitted after the
-/// function body); because the function is not yet compiled when constants are inserted,
-/// [`set_offset`](crate::ir::ConstantPool::set_offset) must be called once a constant's offset
-/// from the beginning of the function is known (see
-/// `relaxation` in `relaxation.rs`).
-#[derive(Clone)]
-#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
-pub struct ConstantPoolEntry {
-    data: ConstantData,
-    offset: Option<ConstantOffset>,
-}
-
-impl ConstantPoolEntry {
-    fn new(data: ConstantData) -> Self {
-        Self { data, offset: None }
-    }
-
-    /// Return the size of the constant at this entry.
-    pub fn len(&self) -> usize {
-        self.data.len()
-    }
-
-    /// Assign a new offset to the constant at this entry.
-    pub fn set_offset(&mut self, offset: ConstantOffset) {
-        self.offset = Some(offset)
-    }
-}
-
 /// Maintains the mapping between a constant handle (i.e.  [`Constant`](crate::ir::Constant)) and
 /// its constant data (i.e.  [`ConstantData`](crate::ir::ConstantData)).
 #[derive(Clone)]
@@ -206,7 +174,7 @@ impl ConstantPoolEntry {
 pub struct ConstantPool {
     /// This mapping maintains the insertion order as long as Constants are created with
     /// sequentially increasing integers.
-    handles_to_values: BTreeMap<Constant, ConstantPoolEntry>,
+    handles_to_values: BTreeMap<Constant, ConstantData>,
 
     /// This mapping is unordered (no need for lexicographic ordering) but allows us to map
     /// constant data back to handles.
@@ -244,64 +212,34 @@ impl ConstantPool {
     /// Retrieve the constant data given a handle.
     pub fn get(&self, constant_handle: Constant) -> &ConstantData {
         assert!(self.handles_to_values.contains_key(&constant_handle));
-        &self.handles_to_values.get(&constant_handle).unwrap().data
+        self.handles_to_values.get(&constant_handle).unwrap()
     }
 
     /// Link a constant handle to its value. This does not de-duplicate data but does avoid
     /// replacing any existing constant values. use `set` to tie a specific `const42` to its value;
     /// use `insert` to add a value and return the next available `const` entity.
     pub fn set(&mut self, constant_handle: Constant, constant_value: ConstantData) {
-        let replaced = self.handles_to_values.insert(
+        let replaced = self
-            constant_handle,
+            .handles_to_values
-            ConstantPoolEntry::new(constant_value.clone()),
+            .insert(constant_handle, constant_value.clone());
-        );
         assert!(
             replaced.is_none(),
             "attempted to overwrite an existing constant {:?}: {:?} => {:?}",
             constant_handle,
             &constant_value,
-            replaced.unwrap().data
+            replaced.unwrap()
         );
         self.values_to_handles
             .insert(constant_value, constant_handle);
     }
 
-    /// Assign an offset to a given constant, where the offset is the number of bytes from the
-    /// beginning of the function to the beginning of the constant data inside the pool.
-    pub fn set_offset(&mut self, constant_handle: Constant, constant_offset: ConstantOffset) {
-        assert!(
-            self.handles_to_values.contains_key(&constant_handle),
-            "A constant handle must have already been inserted into the pool; perhaps a \
-             constant pool was created outside of the pool?"
-        );
-        self.handles_to_values
-            .entry(constant_handle)
-            .and_modify(|e| e.offset = Some(constant_offset));
-    }
-
-    /// Retrieve the offset of a given constant, where the offset is the number of bytes from the
-    /// beginning of the function to the beginning of the constant data inside the pool.
-    pub fn get_offset(&self, constant_handle: Constant) -> ConstantOffset {
-        self.handles_to_values
-            .get(&constant_handle)
-            .expect(
-                "A constant handle must have a corresponding constant value; was a constant \
-                 handle created outside of the pool?",
-            )
-            .offset
-            .expect(
-                "A constant offset has not yet been set; verify that `set_offset` has been \
-                 called before this point",
-            )
-    }
-
     /// Iterate over the constants in insertion order.
     pub fn iter(&self) -> impl Iterator<Item = (&Constant, &ConstantData)> {
-        self.handles_to_values.iter().map(|(h, e)| (h, &e.data))
+        self.handles_to_values.iter()
     }
 
     /// Iterate over mutable entries in the constant pool in insertion order.
-    pub fn entries_mut(&mut self) -> impl Iterator<Item = &mut ConstantPoolEntry> {
+    pub fn entries_mut(&mut self) -> impl Iterator<Item = &mut ConstantData> {
         self.handles_to_values.values_mut()
     }
 
@@ -398,22 +336,6 @@ mod tests {
         sut.get(a); // panics, only use constants returned by ConstantPool
     }
 
-    #[test]
-    fn get_offset() {
-        let mut sut = ConstantPool::new();
-        let a = sut.insert(vec![1].into());
-        sut.set_offset(a, 42);
-        assert_eq!(sut.get_offset(a), 42)
-    }
-
-    #[test]
-    #[should_panic]
-    fn get_nonexistent_offset() {
-        let mut sut = ConstantPool::new();
-        let a = sut.insert(vec![1].into());
-        sut.get_offset(a); // panics, set_offset should have been called
-    }
-
     #[test]
     fn display_constant_data() {
         assert_eq!(ConstantData::from([0].as_ref()).to_string(), "0x00");

cranelift/codegen/src/ir/mod.rs

@@ -31,7 +31,7 @@ pub use crate::ir::atomic_rmw_op::AtomicRmwOp;
 pub use crate::ir::builder::{
     InsertBuilder, InstBuilder, InstBuilderBase, InstInserterBase, ReplaceBuilder,
 };
-pub use crate::ir::constant::{ConstantData, ConstantOffset, ConstantPool};
+pub use crate::ir::constant::{ConstantData, ConstantPool};
 pub use crate::ir::dfg::{DataFlowGraph, ValueDef};
 pub use crate::ir::entities::{
     Block, Constant, FuncRef, GlobalValue, Heap, Immediate, Inst, JumpTable, SigRef, StackSlot,

cranelift/codegen/src/isa/test_utils.rs

@@ -2,7 +2,7 @@
 #![allow(dead_code)]
 
 use crate::binemit::{Addend, CodeOffset, CodeSink, Reloc};
-use crate::ir::{ConstantOffset, ExternalName, Opcode, SourceLoc, TrapCode};
+use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode};
 
 use alloc::vec::Vec;
 use std::string::String;
@@ -66,8 +66,6 @@ impl CodeSink for TestCodeSink {
     ) {
     }
 
-    fn reloc_constant(&mut self, _rel: Reloc, _constant_offset: ConstantOffset) {}
-
     fn trap(&mut self, _code: TrapCode, _srcloc: SourceLoc) {}
 
     fn begin_jumptables(&mut self) {}

cranelift/codegen/src/machinst/buffer.rs

@@ -1641,7 +1641,6 @@ impl<I: VCodeInst> TextSectionBuilder for MachTextSectionBuilder<I> {
 #[cfg(all(test, feature = "arm64"))]
 mod test {
     use super::*;
-    use crate::ir::ConstantOffset;
     use crate::isa::aarch64::inst::xreg;
     use crate::isa::aarch64::inst::{BranchTarget, CondBrKind, EmitInfo, Inst};
     use crate::machinst::MachInstEmit;
@@ -2067,7 +2066,6 @@ mod test {
             fn reloc_external(&mut self, _: SourceLoc, r: Reloc, _: &ExternalName, _: Addend) {
                 self.relocs.push((self.offset, r));
             }
-            fn reloc_constant(&mut self, _: Reloc, _: ConstantOffset) {}
             fn trap(&mut self, t: TrapCode, _: SourceLoc) {
                 self.traps.push((self.offset, t));
             }

cranelift/filetests/src/test_compile.rs

@@ -95,7 +95,6 @@ impl binemit::CodeSink for SizeSink {
         _addend: binemit::Addend,
     ) {
     }
-    fn reloc_constant(&mut self, _: binemit::Reloc, _: ir::ConstantOffset) {}
     fn trap(&mut self, _code: ir::TrapCode, _srcloc: ir::SourceLoc) {}
     fn begin_jumptables(&mut self) {}
     fn begin_rodata(&mut self) {}

cranelift/jit/src/backend.rs

@@ -893,16 +893,4 @@ impl RelocSink for JITRelocSink {
             addend,
         });
     }
-
-    fn reloc_constant(&mut self, _offset: CodeOffset, reloc: Reloc, _constant: ir::ConstantOffset) {
-        match reloc {
-            Reloc::X86PCRelRodata4 => {
-                // Not necessary to record this unless we are going to split apart code and its
-                // jumptbl/rodata.
-            }
-            _ => {
-                panic!("Unhandled reloc");
-            }
-        }
-    }
 }

cranelift/object/src/backend.rs

@@ -734,16 +734,4 @@ impl RelocSink for ObjectRelocSink {
             name: name.clone(),
         })
     }
-
-    fn reloc_constant(&mut self, _offset: CodeOffset, reloc: Reloc, _jt: ir::ConstantOffset) {
-        match reloc {
-            Reloc::X86PCRelRodata4 => {
-                // Not necessary to record this unless we are going to split apart code and its
-                // jumptbl/rodata.
-            }
-            _ => {
-                panic!("Unhandled reloc");
-            }
-        }
-    }
 }

cranelift/src/disasm.rs

@@ -36,22 +36,6 @@ impl binemit::RelocSink for PrintRelocs {
             .unwrap();
         }
     }
-
-    fn reloc_constant(
-        &mut self,
-        code_offset: binemit::CodeOffset,
-        reloc: binemit::Reloc,
-        constant: ir::ConstantOffset,
-    ) {
-        if self.flag_print {
-            writeln!(
-                &mut self.text,
-                "reloc_constant: {} {} at {}",
-                reloc, constant, code_offset
-            )
-            .unwrap();
-        }
-    }
 }
 
 pub struct PrintTraps {

crates/cranelift/src/compiler.rs

@@ -643,16 +643,6 @@ impl binemit::RelocSink for RelocSink {
             addend,
         });
     }
-
-    fn reloc_constant(
-        &mut self,
-        _code_offset: binemit::CodeOffset,
-        _reloc: binemit::Reloc,
-        _constant_offset: ir::ConstantOffset,
-    ) {
-        // Do nothing for now: cranelift emits constant data after the function code and also emits
-        // function code with correct relative offsets to the constant data.
-    }
 }
 
 impl RelocSink {
@@ -764,12 +754,4 @@ impl binemit::RelocSink for TrampolineRelocSink {
             addend,
         });
     }
-    fn reloc_constant(
-        &mut self,
-        _code_offset: binemit::CodeOffset,
-        _reloc: binemit::Reloc,
-        _constant_offset: ir::ConstantOffset,
-    ) {
-        panic!("trampoline compilation should not produce constant relocs");
-    }
 }

crates/cranelift/src/obj.rs

@@ -310,16 +310,6 @@ impl<'a> ObjectBuilder<'a> {
                 Reloc::Abs4 => (RelocationKind::Absolute, RelocationEncoding::Generic, 32),
                 Reloc::Abs8 => (RelocationKind::Absolute, RelocationEncoding::Generic, 64),
 
-                // This is emitted by the old x86 backend and is only present
-                // for when the constant rodata is separated from the code
-                // itself. We don't do that, though, so we ignore these
-                // relocations since the offsets already listed here are already
-                // correct.
-                //
-                // FIXME(#3009): when the old backend is removed delete this
-                // case.
-                Reloc::X86PCRelRodata4 => continue,
-
                 other => unimplemented!("Unimplemented relocation {:?}", other),
             };
             self.obj