From 49bab6db7f7d32456e79c8b865bccf6481bf9e57 Mon Sep 17 00:00:00 2001
From: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Date: Mon, 20 Mar 2023 20:20:00 +0100
Subject: [PATCH] Ensure the sequence number doesn't leak out of Layout (#6061)

Previously it could affect the PartialEq and Hash impls. Ignoring the
sequence number in PartialEq and Hash allows us to not renumber all
blocks in the incremental cache.
---
 cranelift/codegen/src/incremental_cache.rs | 18 +++---------------
 cranelift/codegen/src/ir/layout.rs         | 22 ++++++++++++++++++++--
 fuzz/fuzz_targets/cranelift-icache.rs      |  4 ++--
 3 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/cranelift/codegen/src/incremental_cache.rs b/cranelift/codegen/src/incremental_cache.rs
index 8e72d5e76e..fdd7ca1615 100644
--- a/cranelift/codegen/src/incremental_cache.rs
+++ b/cranelift/codegen/src/incremental_cache.rs
@@ -44,7 +44,7 @@ impl Context {
         let cache_key_hash = {
             let _tt = timing::try_incremental_cache();
 
-            let cache_key_hash = compute_cache_key(isa, &mut self.func);
+            let cache_key_hash = compute_cache_key(isa, &self.func);
 
             if let Some(blob) = cache_store.get(&cache_key_hash.0) {
                 match try_finish_recompile(&self.func, &blob) {
@@ -162,19 +162,7 @@ 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 mut Function) -> Self {
-        // Make sure the blocks and instructions are sequenced the same way as we might
-        // have serialized them earlier. This is the symmetric of what's done in
-        // `try_load`.
-        let mut block = f.stencil.layout.entry_block().expect("Missing entry block");
-        loop {
-            f.stencil.layout.full_block_renumber(block);
-            if let Some(next_block) = f.stencil.layout.next_block(block) {
-                block = next_block;
-            } else {
-                break;
-            }
-        }
+    fn new(isa: &dyn TargetIsa, f: &'a Function) -> Self {
         CacheKey {
             stencil: &f.stencil,
             parameters: CompileParameters::from_isa(isa),
@@ -185,7 +173,7 @@ impl<'a> CacheKey<'a> {
 /// Compute a cache key, and hash it on your behalf.
 ///
 /// Since computing the `CacheKey` is a bit expensive, it should be done as least as possible.
-pub fn compute_cache_key(isa: &dyn TargetIsa, func: &mut Function) -> CacheKeyHash {
+pub fn compute_cache_key(isa: &dyn TargetIsa, func: &Function) -> CacheKeyHash {
     use core::hash::{Hash as _, Hasher};
     use sha2::Digest as _;
 
diff --git a/cranelift/codegen/src/ir/layout.rs b/cranelift/codegen/src/ir/layout.rs
index 1e5d73dc2b..f04560e243 100644
--- a/cranelift/codegen/src/ir/layout.rs
+++ b/cranelift/codegen/src/ir/layout.rs
@@ -209,7 +209,7 @@ impl Layout {
     ///
     /// This doesn't affect the position of anything, but it gives more room in the internal
     /// sequence numbers for inserting instructions later.
-    pub(crate) fn full_block_renumber(&mut self, block: Block) {
+    fn full_block_renumber(&mut self, block: Block) {
         let _tt = timing::layout_renumber();
         // Avoid 0 as this is reserved for the program point indicating the block itself
         let mut seq = MAJOR_STRIDE;
@@ -599,7 +599,7 @@ impl Layout {
     }
 }
 
-#[derive(Clone, Debug, Default, PartialEq, Hash)]
+#[derive(Clone, Debug, Default)]
 struct InstNode {
     /// The Block containing this instruction, or `None` if the instruction is not yet inserted.
     block: PackedOption<Block>,
@@ -608,6 +608,24 @@ struct InstNode {
     seq: SequenceNumber,
 }
 
+impl PartialEq for InstNode {
+    fn eq(&self, other: &Self) -> bool {
+        // Ignore the sequence number as it is an optimization used by pp_cmp and may be different
+        // even for equivalent layouts.
+        self.block == other.block && self.prev == other.prev && self.next == other.next
+    }
+}
+
+impl core::hash::Hash for InstNode {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        // Ignore the sequence number as it is an optimization used by pp_cmp and may be different
+        // even for equivalent layouts.
+        self.block.hash(state);
+        self.prev.hash(state);
+        self.next.hash(state);
+    }
+}
+
 /// Iterate over instructions in a block in layout order. See `Layout::block_insts()`.
 pub struct Insts<'f> {
     layout: &'f Layout,
diff --git a/fuzz/fuzz_targets/cranelift-icache.rs b/fuzz/fuzz_targets/cranelift-icache.rs
index 4c0ca54553..886d158ac1 100644
--- a/fuzz/fuzz_targets/cranelift-icache.rs
+++ b/fuzz/fuzz_targets/cranelift-icache.rs
@@ -103,7 +103,7 @@ impl<'a> Arbitrary<'a> for FunctionWithIsa {
 fuzz_target!(|func: FunctionWithIsa| {
     let FunctionWithIsa { mut func, isa } = func;
 
-    let cache_key_hash = icache::compute_cache_key(&*isa, &mut func);
+    let cache_key_hash = icache::compute_cache_key(&*isa, &func);
 
     let mut context = Context::for_function(func.clone());
     let prev_stencil = match context.compile_stencil(&*isa) {
@@ -187,7 +187,7 @@ fuzz_target!(|func: FunctionWithIsa| {
         false
     };
 
-    let new_cache_key_hash = icache::compute_cache_key(&*isa, &mut func);
+    let new_cache_key_hash = icache::compute_cache_key(&*isa, &func);
 
     if expect_cache_hit {
         assert!(cache_key_hash == new_cache_key_hash);