Use precise alias regions for statically-known entity imports

cranelift/codegen/src/alias_analysis.rs

@@ -424,6 +424,32 @@ impl LastStores {
         }
     }
 
+    /// Roll this state back to the memory version from just before `dead`,
+    /// which is a store being removed from the function by dead-store
+    /// elimination.
+    ///
+    /// `prev_last_store` must be the last-store instruction that immediately
+    /// preceded `dead`, as recorded when `dead` itself was processed.
+    ///
+    /// Only `dead`'s own alias-region slot is restored. A store with no alias
+    /// region is treated as a fence by `update`, which clears *every* region
+    /// slot, and we do not undo that; in that case, we leave this state
+    /// alone. Similarly, stores marked observed while processing `dead` stay
+    /// observed.
+    fn undo_store(&mut self, func: &Function, dead: Inst, prev_last_store: PackedOption<Inst>) {
+        debug_assert!(func.dfg.insts[dead].opcode().can_store());
+
+        let Some(region) = func.dfg.insts[dead].alias_region(&func.dfg) else {
+            return;
+        };
+
+        // Only roll back if `dead` really is the current last store to its
+        // region.
+        if self.regions[region].expand() == Some(dead) {
+            self.regions[region] = prev_last_store;
+        }
+    }
+
     /// Get the last-store instruction for the given `inst`'s alias region, if
     /// any.
     fn get_last_store(&self, func: &Function, inst: Inst) -> PackedOption<Inst> {
@@ -527,6 +553,26 @@ struct MemoryLoc {
     extending_opcode: Option<Opcode>,
 }
 
+/// What is known to be in memory at an associated `MemoryLoc`.
+#[derive(Clone, Copy, Debug)]
+struct KnownValue {
+    /// The value held at the associated `MemoryLoc`.
+    value: Value,
+
+    /// The instruction that produced `value`: either the load that read it out
+    /// of memory or the store that wrote it there.
+    ///
+    /// Kept around for quick dominance checks.
+    def_inst: Inst,
+
+    /// When this entry was created by a store, the last-store instruction that
+    /// immediately preceded that store: that is, the memory version this
+    /// location was at just *before* `def_inst` overwrote it.
+    ///
+    /// `None` for entries created by loads.
+    prev_last_store: Option<PackedOption<Inst>>,
+}
+
 /// The result of processing an instruction through alias analysis.
 pub enum OptResult {
     /// No optimization applied.
@@ -576,9 +622,7 @@ pub struct AliasAnalysis<'a> {
     /// Known memory-value equivalences. This is the result of the
     /// analysis. This is a mapping from (last store, address
     /// expression, offset, type) to SSA `Value`.
-    ///
-    /// We keep the defining inst around for quick dominance checks.
-    mem_values: FxHashMap<MemoryLoc, (Inst, Value)>,
+    mem_values: FxHashMap<MemoryLoc, KnownValue>,
 }
 
 impl<'a> AliasAnalysis<'a> {
@@ -754,7 +798,36 @@ impl<'a> AliasAnalysis<'a> {
                             ty,
                             extending_opcode: get_ext_opcode(opcode),
                         };
-                        self.mem_values.remove(&dead_loc);
+                        let dead_entry = self.mem_values.remove(&dead_loc);
+
+                        // Roll our last-store state back to the memory version
+                        // just before the dead store, so that `state` describes
+                        // memory as if the dead store had never happened.
+                        //
+                        // Our callers remove the dead store from the layout and
+                        // then reprocess this overwriting store. Without the
+                        // rollback, that reprocessing keys its `mem_values`
+                        // lookup on the instruction we just removed, finds
+                        // nothing, and so fails to notice that the overwriter
+                        // has now become an idempotent store. Chains like
+                        //
+                        //     v1 = load.i32 region0 v0
+                        //     store region0 v2, v0  ;; dead
+                        //     store region0 v1, v0  ;; idempotent, once the
+                        //                           ;; dead store is gone
+                        //
+                        // would then need a whole additional pass over the
+                        // function to collapse each link.
+                        //
+                        // A missing entry means we never processed the dead
+                        // store as a store in this pass (it can come from a
+                        // precomputed `block_input` snapshot, for a predecessor
+                        // block we have not walked yet), so we have no previous
+                        // version to roll back to and simply don't.
+                        if let Some(prev) = dead_entry.and_then(|e| e.prev_last_store) {
+                            state.undo_store(func, last_store, prev);
+                        }
+
                         return OptResult::DeadStore {
                             dead: last_store,
                             overwriter: inst,
@@ -769,7 +842,12 @@ impl<'a> AliasAnalysis<'a> {
                     ty,
                     extending_opcode: get_ext_opcode(opcode),
                 };
-                if let Some((def_inst, known_value)) = self.mem_values.get(&check_loc).cloned() {
+                if let Some(KnownValue {
+                    def_inst,
+                    value: known_value,
+                    ..
+                }) = self.mem_values.get(&check_loc).cloned()
+                {
                     // Check for idempotent stores, where we are
                     // storing the exact same value back to a location
                     // that already has that value.
@@ -806,7 +884,14 @@ impl<'a> AliasAnalysis<'a> {
                     extending_opcode: get_ext_opcode(opcode),
                 };
                 trace!("  --> updating known values in memory: {mem_loc:?} = {store_data}");
-                self.mem_values.insert(mem_loc, (inst, store_data));
+                self.mem_values.insert(
+                    mem_loc,
+                    KnownValue {
+                        def_inst: inst,
+                        value: store_data,
+                        prev_last_store: Some(last_store),
+                    },
+                );
 
                 OptResult::None
             } else if opcode.can_load() {
@@ -831,8 +916,9 @@ impl<'a> AliasAnalysis<'a> {
                 // load (stores will always dominate though if
                 // their `last_store` survives through
                 // meet-points to this use-site).
-                let aliased = if let Some((def_inst, value)) =
-                    self.mem_values.get(&mem_loc).cloned()
+                let aliased = if let Some(KnownValue {
+                    def_inst, value, ..
+                }) = self.mem_values.get(&mem_loc).cloned()
                 {
                     trace!("  see known value {value} from {def_inst}");
                     if self.domtree.dominates(def_inst, inst, &func.layout) {
@@ -851,7 +937,16 @@ impl<'a> AliasAnalysis<'a> {
                 // as a new equivalent value.
                 if aliased.is_none() {
                     trace!("  --> inserting load result {load_result} at loc {mem_loc:?}");
-                    self.mem_values.insert(mem_loc, (inst, load_result));
+                    self.mem_values.insert(
+                        mem_loc,
+                        KnownValue {
+                            def_inst: inst,
+                            value: load_result,
+                            // A load does not advance the memory version, so
+                            // there is no previous version to roll back to.
+                            prev_last_store: None,
+                        },
+                    );
                 }
 
                 match aliased {

cranelift/codegen/src/inst_predicates.rs

@@ -94,11 +94,27 @@ pub fn is_mergeable_for_egraph(func: &Function, inst: Inst) -> bool {
 
 /// Does the given instruction have any side-effect as per [has_side_effect], or else is a load,
 /// but not the get_pinned_reg opcode?
+///
+/// Loads are included so that lowering colors them as side-effecting, which is
+/// what keeps a load from being merged into a consumer across an intervening
+/// store. Deciding whether a load has to be *emitted* is a different question;
+/// see [`must_lower_even_if_unused`].
 pub fn has_lowering_side_effect(func: &Function, inst: Inst) -> bool {
     let op = func.dfg.insts[inst].opcode();
     op != Opcode::GetPinnedReg && (has_side_effect(func, inst) || op.can_load())
 }
 
+/// Must lowering emit the given instruction even when none of its results are
+/// used?
+///
+/// This is [has_lowering_side_effect] without its "or is a load" clause: a load
+/// that is defined not to trap has no effect of its own, so if nothing wants the
+/// value it read there is no reason to emit it.
+pub fn must_lower_even_if_unused(func: &Function, inst: Inst) -> bool {
+    let op = func.dfg.insts[inst].opcode();
+    op != Opcode::GetPinnedReg && has_side_effect(func, inst)
+}
+
 /// Is the given instruction a constant value (`iconst`, `fconst`) that can be
 /// represented in 64 bits?
 pub fn is_constant_64bit(func: &Function, inst: Inst) -> Option<u64> {

cranelift/codegen/src/machinst/lower.rs

@@ -6,7 +6,9 @@
 // top of it, e.g. the side-effect/coloring analysis and the scan support.
 
 use crate::entity::SecondaryMap;
-use crate::inst_predicates::{has_lowering_side_effect, is_constant_64bit};
+use crate::inst_predicates::{
+    has_lowering_side_effect, is_constant_64bit, must_lower_even_if_unused,
+};
 use crate::ir::{
     ArgumentPurpose, Block, BlockArg, Constant, ConstantData, DataFlowGraph, ExternalName,
     Function, GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, RelSourceLoc, SigRef,
@@ -750,14 +752,19 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
 
             // Are any outputs used at least once?
             let value_needed = self.is_any_inst_result_needed(inst);
+
+            // Do we have to emit this instruction even though nothing uses its
+            // results? Note that this is not the same question as
+            // `has_side_effect` above: loads are colored as side-effecting so
+            // that load merging cannot move one across a store, but a load that
+            // is defined not to trap can simply be dropped when it is dead.
+            let must_lower = must_lower_even_if_unused(self.f, inst);
+
             trace!(
-                "lower_clif_block: block {} inst {} ({:?}) is_branch {} side_effect {} value_needed {}",
-                block,
-                inst,
-                data,
+                "lower_clif_block: {block}, {inst}, ({data:?}), is_branch {}, \
+                 has_side_effect {has_side_effect}, must_lower {must_lower}, \
+                 value_needed {value_needed}",
                 data.opcode().is_branch(),
-                has_side_effect,
-                value_needed,
             );
 
             // Update scan state to color prior to this inst (as we are scanning
@@ -777,11 +784,11 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
             // order, and therefore **before** in reversed order.
             // Only emit value label aliases if the instruction will be lowered
             // (otherwise we want to keep using the earlier label instead).
-            self.emit_value_label_live_range_start_for_inst(inst, has_side_effect || value_needed);
+            self.emit_value_label_live_range_start_for_inst(inst, must_lower || value_needed);
 
             // Normal instruction: codegen if the instruction is side-effecting
             // or any of its outputs is used.
-            if has_side_effect || value_needed {
+            if must_lower || value_needed {
                 trace!("lowering: inst {}: {}", inst, self.f.dfg.display_inst(inst));
                 let temp_regs = match backend.lower(self, inst) {
                     Some(regs) => regs,

cranelift/filetests/filetests/alias/check-unset-reset-flag.clif

@@ -21,7 +21,7 @@ block0(v0: i64, v1: i32):
 ; block0(v0: i64, v1: i32):
 ;     v2 = load.i64 notrap aligned region0 v0
 ;     trapz v2, user42
-;     store notrap aligned region0 v2, v0
 ;     v4 = iadd v1, v1
 ;     return v4
 ; }
+