Couple of optimizations to the Cranelift incremental cache (#11186)

* Fix a couple of comments

* Remove flags.predicate_view()

It is a remenant of the old backend framework.

* Avoid string conversions for hashing the TargetIsa

* Remove func_body_len

It is identical to buffer.data.len()

* Introduce IsaFlagsHashKey

Author: bjorn3

cranelift/codegen/meta/src/gen_settings.rs

@@ -208,18 +208,6 @@ fn gen_getters(group: &SettingGroup, fmt: &mut Formatter) {
     fmt.doc_comment("User-defined settings.");
     fmtln!(fmt, "#[allow(dead_code, reason = \"generated code\")]");
     fmt.add_block("impl Flags", |fmt| {
-        fmt.doc_comment("Get a view of the boolean predicates.");
-        fmt.add_block(
-            "pub fn predicate_view(&self) -> crate::settings::PredicateView<'_>",
-            |fmt| {
-                fmtln!(
-                    fmt,
-                    "crate::settings::PredicateView::new(&self.bytes[{}..])",
-                    group.bool_start_byte_offset
-                );
-            },
-        );
-
         if !group.settings.is_empty() {
             fmt.doc_comment("Dynamic numbered predicate getter.");
             fmt.add_block("fn numbered_predicate(&self, p: usize) -> bool", |fmt| {
@@ -424,9 +412,18 @@ fn gen_display(group: &SettingGroup, fmt: &mut Formatter) {
     });
 }
 
+fn gen_hash_key(fmt: &mut Formatter) {
+    fmt.add_block("impl Flags", |fmt| {
+        fmt.doc_comment("Get the flag values as raw bytes for hashing.");
+        fmt.add_block("pub fn hash_key(&self) -> &[u8]", |fmt| {
+            fmtln!(fmt, "&self.bytes");
+        });
+    });
+}
+
 fn gen_group(group: &SettingGroup, parent: ParentGroup, fmt: &mut Formatter) {
     // Generate struct.
-    fmtln!(fmt, "#[derive(Clone, Hash)]");
+    fmtln!(fmt, "#[derive(Clone, PartialEq, Hash)]");
     fmt.doc_comment(format!("Flags group `{}`.", group.name));
     fmt.add_block("pub struct Flags", |fmt| {
         fmtln!(fmt, "bytes: [u8; {}],", group.byte_size());
@@ -439,6 +436,7 @@ fn gen_group(group: &SettingGroup, parent: ParentGroup, fmt: &mut Formatter) {
     gen_descriptors(group, fmt);
     gen_template(group, fmt);
     gen_display(group, fmt);
+    gen_hash_key(fmt);
 }
 
 pub(crate) fn generate(

cranelift/codegen/src/incremental_cache.rs

@@ -21,17 +21,16 @@
 //! the above three primitives to form a full incremental caching system.
 
 use core::fmt;
+use core::hash::{Hash, Hasher};
 
-use crate::alloc::string::String;
 use crate::alloc::vec::Vec;
 use crate::ir::Function;
 use crate::ir::function::{FunctionStencil, VersionMarker};
 use crate::machinst::{CompiledCode, CompiledCodeStencil};
 use crate::result::CompileResult;
 use crate::{CompileError, Context, trace};
 use crate::{isa::TargetIsa, timing};
-use alloc::borrow::{Cow, ToOwned as _};
-use alloc::string::ToString as _;
+use alloc::borrow::Cow;
 use cranelift_control::ControlPlane;
 
 impl Context {
@@ -133,43 +132,29 @@ struct CachedFunc {
 ///
 /// Note: the key will be invalidated across different versions of cranelift, as the
 /// `FunctionStencil` contains a `VersionMarker` itself.
-#[derive(Hash)]
 struct CacheKey<'a> {
     stencil: &'a FunctionStencil,
-    parameters: CompileParameters,
+    isa: &'a dyn TargetIsa,
 }
 
-#[derive(Clone, PartialEq, Hash, serde_derive::Serialize, serde_derive::Deserialize)]
-struct CompileParameters {
-    isa: String,
-    triple: String,
-    flags: String,
-    isa_flags: Vec<String>,
-}
-
-impl CompileParameters {
-    fn from_isa(isa: &dyn TargetIsa) -> Self {
-        Self {
-            isa: isa.name().to_owned(),
-            triple: isa.triple().to_string(),
-            flags: isa.flags().to_string(),
-            isa_flags: isa
-                .isa_flags()
-                .into_iter()
-                .map(|v| v.value_string())
-                .collect(),
-        }
+impl<'a> Hash for CacheKey<'a> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.stencil.hash(state);
+        self.isa.name().hash(state);
+        self.isa.triple().hash(state);
+        self.isa.flags().hash(state);
+        self.isa.isa_flags_hash_key().hash(state);
     }
 }
 
 impl<'a> CacheKey<'a> {
     /// Creates a new cache store key for a function.
     ///
     /// This is a bit expensive to compute, so it should be cached and reused as much as possible.
-    fn new(isa: &dyn TargetIsa, f: &'a Function) -> Self {
+    fn new(isa: &'a dyn TargetIsa, f: &'a Function) -> Self {
         CacheKey {
             stencil: &f.stencil,
-            parameters: CompileParameters::from_isa(isa),
+            isa,
         }
     }
 }

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

@@ -5,7 +5,7 @@ use crate::ir::{self, Function, Type};
 use crate::isa::aarch64::settings as aarch64_settings;
 #[cfg(feature = "unwind")]
 use crate::isa::unwind::systemv;
-use crate::isa::{Builder as IsaBuilder, FunctionAlignment, TargetIsa};
+use crate::isa::{Builder as IsaBuilder, FunctionAlignment, IsaFlagsHashKey, TargetIsa};
 use crate::machinst::{
     CompiledCode, CompiledCodeStencil, MachInst, MachTextSectionBuilder, Reg, SigSet,
     TextSectionBuilder, VCode, compile,
@@ -112,6 +112,10 @@ impl TargetIsa for AArch64Backend {
         self.isa_flags.iter().collect()
     }
 
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_> {
+        IsaFlagsHashKey(self.isa_flags.hash_key())
+    }
+
     fn is_branch_protection_enabled(&self) -> bool {
         self.isa_flags.use_bti()
     }

cranelift/codegen/src/isa/mod.rs

@@ -294,6 +294,9 @@ pub trait TargetIsa: fmt::Display + Send + Sync {
     /// Get the ISA-dependent flag values that were used to make this trait object.
     fn isa_flags(&self) -> Vec<settings::Value>;
 
+    /// Get the ISA-dependent flag values as raw bytes for hashing.
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_>;
+
     /// Get a flag indicating whether branch protection is enabled.
     fn is_branch_protection_enabled(&self) -> bool {
         false
@@ -415,6 +418,10 @@ pub trait TargetIsa: fmt::Display + Send + Sync {
     fn default_argument_extension(&self) -> ir::ArgumentExtension;
 }
 
+/// A wrapper around the ISA-dependent flags types which only implements `Hash`.
+#[derive(Hash)]
+pub struct IsaFlagsHashKey<'a>(&'a [u8]);
+
 /// Function alignment specifications as required by an ISA, returned by
 /// [`TargetIsa::function_alignment`].
 #[derive(Copy, Clone)]

cranelift/codegen/src/isa/pulley_shared/mod.rs

@@ -11,7 +11,7 @@ use crate::{
     MachTextSectionBuilder, TextSectionBuilder,
     dominator_tree::DominatorTree,
     ir,
-    isa::{self, OwnedTargetIsa, TargetIsa},
+    isa::{self, IsaFlagsHashKey, OwnedTargetIsa, TargetIsa},
     machinst::{self, CompiledCodeStencil, MachInst, SigSet, VCode},
     result::CodegenResult,
     settings::{self as shared_settings, Flags},
@@ -155,6 +155,10 @@ where
         self.isa_flags.iter().collect()
     }
 
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_> {
+        IsaFlagsHashKey(self.isa_flags.hash_key())
+    }
+
     fn dynamic_vector_bytes(&self, _dynamic_ty: ir::Type) -> u32 {
         512
     }

cranelift/codegen/src/isa/riscv64/mod.rs

@@ -3,7 +3,9 @@
 use crate::dominator_tree::DominatorTree;
 use crate::ir::{Function, Type};
 use crate::isa::riscv64::settings as riscv_settings;
-use crate::isa::{Builder as IsaBuilder, FunctionAlignment, OwnedTargetIsa, TargetIsa};
+use crate::isa::{
+    Builder as IsaBuilder, FunctionAlignment, IsaFlagsHashKey, OwnedTargetIsa, TargetIsa,
+};
 use crate::machinst::{
     CompiledCode, CompiledCodeStencil, MachInst, MachTextSectionBuilder, Reg, SigSet,
     TextSectionBuilder, VCode, compile,
@@ -114,6 +116,10 @@ impl TargetIsa for Riscv64Backend {
         self.isa_flags.iter().collect()
     }
 
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_> {
+        IsaFlagsHashKey(self.isa_flags.hash_key())
+    }
+
     #[cfg(feature = "unwind")]
     fn emit_unwind_info(
         &self,

cranelift/codegen/src/isa/s390x/mod.rs

@@ -5,7 +5,7 @@ use crate::ir::{self, Function, Type};
 use crate::isa::s390x::settings as s390x_settings;
 #[cfg(feature = "unwind")]
 use crate::isa::unwind::systemv::RegisterMappingError;
-use crate::isa::{Builder as IsaBuilder, FunctionAlignment, TargetIsa};
+use crate::isa::{Builder as IsaBuilder, FunctionAlignment, IsaFlagsHashKey, TargetIsa};
 use crate::machinst::{
     CompiledCode, CompiledCodeStencil, MachInst, MachTextSectionBuilder, Reg, SigSet,
     TextSectionBuilder, VCode, compile,
@@ -112,6 +112,10 @@ impl TargetIsa for S390xBackend {
         self.isa_flags.iter().collect()
     }
 
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_> {
+        IsaFlagsHashKey(self.isa_flags.hash_key())
+    }
+
     fn dynamic_vector_bytes(&self, _dyn_ty: Type) -> u32 {
         16
     }

cranelift/codegen/src/isa/x64/mod.rs

@@ -8,7 +8,7 @@ use crate::ir::{self, Function, Type, types};
 #[cfg(feature = "unwind")]
 use crate::isa::unwind::systemv;
 use crate::isa::x64::settings as x64_settings;
-use crate::isa::{Builder as IsaBuilder, FunctionAlignment};
+use crate::isa::{Builder as IsaBuilder, FunctionAlignment, IsaFlagsHashKey};
 use crate::machinst::{
     CompiledCode, CompiledCodeStencil, MachInst, MachTextSectionBuilder, Reg, SigSet,
     TextSectionBuilder, VCode, compile,
@@ -104,6 +104,10 @@ impl TargetIsa for X64Backend {
         self.x64_flags.iter().collect()
     }
 
+    fn isa_flags_hash_key(&self) -> IsaFlagsHashKey<'_> {
+        IsaFlagsHashKey(self.x64_flags.hash_key())
+    }
+
     fn dynamic_vector_bytes(&self, _dyn_ty: Type) -> u32 {
         16
     }

cranelift/codegen/src/machinst/buffer.rs

@@ -1977,7 +1977,7 @@ pub struct MachSrcLoc<T: CompilePhase> {
     /// section.
     pub start: CodeOffset,
     /// The end of the region of code corresponding to a source location.
-    /// This is relative to the start of the section, not to the start of the
+    /// This is relative to the start of the function, not to the start of the
     /// section.
     pub end: CodeOffset,
     /// The source location.

cranelift/codegen/src/machinst/vcode.rs

@@ -208,16 +208,13 @@ pub struct EmitResult {
     pub buffer: MachBufferFinalized<Stencil>,
 
     /// Offset of each basic block, recorded during emission. Computed
-    /// only if `debug_value_labels` is non-empty.
+    /// only if `machine_code_cfg_info` is enabled.
     pub bb_offsets: Vec<CodeOffset>,
 
     /// Final basic-block edges, in terms of code offsets of
-    /// bb-starts. Computed only if `debug_value_labels` is non-empty.
+    /// bb-starts. Computed only if `machine_code_cfg_info` is enabled.
     pub bb_edges: Vec<(CodeOffset, CodeOffset)>,
 
-    /// Final length of function body.
-    pub func_body_len: CodeOffset,
-
     /// The pretty-printed disassembly, if any. This uses the same
     /// pretty-printing for MachInsts as the pre-regalloc VCode Debug
     /// implementation, but additionally includes the prologue and
@@ -1110,7 +1107,6 @@ impl<I: VCodeInst> VCode<I> {
             buffer: buffer.finish(&self.constants, ctrl_plane),
             bb_offsets,
             bb_edges,
-            func_body_len,
             disasm: if want_disasm { Some(disasm) } else { None },
             sized_stackslot_offsets: self.abi.sized_stackslot_offsets().clone(),
             dynamic_stackslot_offsets: self.abi.dynamic_stackslot_offsets().clone(),