Skip to content

Commit a9238c1

Browse files
fitzgenfitzgen
authored
Cranelift: unwind last-store state after removing a dead store (#14111)
Alias analysis's dead-store elimination removed the dead store's `mem_values` entry, but left the region's last-store slot naming the instruction it had just deleted. Leaving the removed-store meant that when we then reprocess the overwriting store, we keyed its lookup on a removed instruction, found nothing, and failed to notice that (for example) the overwriting store became idempotent and could also be removed. With this commit, each store now records the memory version it displaced, and eliminating a dead store rolls that version back, so a chain like v1 = load.i32 region0 v0 store region0 v2, v0 ;; dead store region0 v1, v0 ;; idempotent once the dead store is gone collapses in the single pass we actually make, rather than removing only one link in the chain and requiring that we do N passes to fully clean up a chain of N dead/idempotent stores. This code pattern the shape fused sync adapters emit around the `MAY_LEAVE` flag and the relevant disas tests each lose a store as a result.
1 parent 899e66b commit a9238c1

8 files changed

Lines changed: 323 additions & 54 deletions

File tree

‎cranelift/codegen/src/alias_analysis.rs‎

Lines changed: 104 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,32 @@ impl LastStores {
424424
}
425425
}
426426

427+
/// Roll this state back to the memory version from just before `dead`,
428+
/// which is a store being removed from the function by dead-store
429+
/// elimination.
430+
///
431+
/// `prev_last_store` must be the last-store instruction that immediately
432+
/// preceded `dead`, as recorded when `dead` itself was processed.
433+
///
434+
/// Only `dead`'s own alias-region slot is restored. A store with no alias
435+
/// region is treated as a fence by `update`, which clears *every* region
436+
/// slot, and we do not undo that; in that case, we leave this state
437+
/// alone. Similarly, stores marked observed while processing `dead` stay
438+
/// observed.
439+
fn undo_store(&mut self, func: &Function, dead: Inst, prev_last_store: PackedOption<Inst>) {
440+
debug_assert!(func.dfg.insts[dead].opcode().can_store());
441+
442+
let Some(region) = func.dfg.insts[dead].alias_region(&func.dfg) else {
443+
return;
444+
};
445+
446+
// Only roll back if `dead` really is the current last store to its
447+
// region.
448+
if self.regions[region].expand() == Some(dead) {
449+
self.regions[region] = prev_last_store;
450+
}
451+
}
452+
427453
/// Get the last-store instruction for the given `inst`'s alias region, if
428454
/// any.
429455
fn get_last_store(&self, func: &Function, inst: Inst) -> PackedOption<Inst> {
@@ -527,6 +553,26 @@ struct MemoryLoc {
527553
extending_opcode: Option<Opcode>,
528554
}
529555

556+
/// What is known to be in memory at an associated `MemoryLoc`.
557+
#[derive(Clone, Copy, Debug)]
558+
struct KnownValue {
559+
/// The value held at the associated `MemoryLoc`.
560+
value: Value,
561+
562+
/// The instruction that produced `value`: either the load that read it out
563+
/// of memory or the store that wrote it there.
564+
///
565+
/// Kept around for quick dominance checks.
566+
def_inst: Inst,
567+
568+
/// When this entry was created by a store, the last-store instruction that
569+
/// immediately preceded that store: that is, the memory version this
570+
/// location was at just *before* `def_inst` overwrote it.
571+
///
572+
/// `None` for entries created by loads.
573+
prev_last_store: Option<PackedOption<Inst>>,
574+
}
575+
530576
/// The result of processing an instruction through alias analysis.
531577
pub enum OptResult {
532578
/// No optimization applied.
@@ -576,9 +622,7 @@ pub struct AliasAnalysis<'a> {
576622
/// Known memory-value equivalences. This is the result of the
577623
/// analysis. This is a mapping from (last store, address
578624
/// expression, offset, type) to SSA `Value`.
579-
///
580-
/// We keep the defining inst around for quick dominance checks.
581-
mem_values: FxHashMap<MemoryLoc, (Inst, Value)>,
625+
mem_values: FxHashMap<MemoryLoc, KnownValue>,
582626
}
583627

584628
impl<'a> AliasAnalysis<'a> {
@@ -754,7 +798,36 @@ impl<'a> AliasAnalysis<'a> {
754798
ty,
755799
extending_opcode: get_ext_opcode(opcode),
756800
};
757-
self.mem_values.remove(&dead_loc);
801+
let dead_entry = self.mem_values.remove(&dead_loc);
802+
803+
// Roll our last-store state back to the memory version
804+
// just before the dead store, so that `state` describes
805+
// memory as if the dead store had never happened.
806+
//
807+
// Our callers remove the dead store from the layout and
808+
// then reprocess this overwriting store. Without the
809+
// rollback, that reprocessing keys its `mem_values`
810+
// lookup on the instruction we just removed, finds
811+
// nothing, and so fails to notice that the overwriter
812+
// has now become an idempotent store. Chains like
813+
//
814+
// v1 = load.i32 region0 v0
815+
// store region0 v2, v0 ;; dead
816+
// store region0 v1, v0 ;; idempotent, once the
817+
// ;; dead store is gone
818+
//
819+
// would then need a whole additional pass over the
820+
// function to collapse each link.
821+
//
822+
// A missing entry means we never processed the dead
823+
// store as a store in this pass (it can come from a
824+
// precomputed `block_input` snapshot, for a predecessor
825+
// block we have not walked yet), so we have no previous
826+
// version to roll back to and simply don't.
827+
if let Some(prev) = dead_entry.and_then(|e| e.prev_last_store) {
828+
state.undo_store(func, last_store, prev);
829+
}
830+
758831
return OptResult::DeadStore {
759832
dead: last_store,
760833
overwriter: inst,
@@ -769,7 +842,12 @@ impl<'a> AliasAnalysis<'a> {
769842
ty,
770843
extending_opcode: get_ext_opcode(opcode),
771844
};
772-
if let Some((def_inst, known_value)) = self.mem_values.get(&check_loc).cloned() {
845+
if let Some(KnownValue {
846+
def_inst,
847+
value: known_value,
848+
..
849+
}) = self.mem_values.get(&check_loc).cloned()
850+
{
773851
// Check for idempotent stores, where we are
774852
// storing the exact same value back to a location
775853
// that already has that value.
@@ -806,7 +884,14 @@ impl<'a> AliasAnalysis<'a> {
806884
extending_opcode: get_ext_opcode(opcode),
807885
};
808886
trace!(" --> updating known values in memory: {mem_loc:?} = {store_data}");
809-
self.mem_values.insert(mem_loc, (inst, store_data));
887+
self.mem_values.insert(
888+
mem_loc,
889+
KnownValue {
890+
def_inst: inst,
891+
value: store_data,
892+
prev_last_store: Some(last_store),
893+
},
894+
);
810895

811896
OptResult::None
812897
} else if opcode.can_load() {
@@ -831,8 +916,9 @@ impl<'a> AliasAnalysis<'a> {
831916
// load (stores will always dominate though if
832917
// their `last_store` survives through
833918
// meet-points to this use-site).
834-
let aliased = if let Some((def_inst, value)) =
835-
self.mem_values.get(&mem_loc).cloned()
919+
let aliased = if let Some(KnownValue {
920+
def_inst, value, ..
921+
}) = self.mem_values.get(&mem_loc).cloned()
836922
{
837923
trace!(" see known value {value} from {def_inst}");
838924
if self.domtree.dominates(def_inst, inst, &func.layout) {
@@ -851,7 +937,16 @@ impl<'a> AliasAnalysis<'a> {
851937
// as a new equivalent value.
852938
if aliased.is_none() {
853939
trace!(" --> inserting load result {load_result} at loc {mem_loc:?}");
854-
self.mem_values.insert(mem_loc, (inst, load_result));
940+
self.mem_values.insert(
941+
mem_loc,
942+
KnownValue {
943+
def_inst: inst,
944+
value: load_result,
945+
// A load does not advance the memory version, so
946+
// there is no previous version to roll back to.
947+
prev_last_store: None,
948+
},
949+
);
855950
}
856951

857952
match aliased {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ block0(v0: i64, v1: i32):
2121
; block0(v0: i64, v1: i32):
2222
; v2 = load.i64 notrap aligned region0 v0
2323
; trapz v2, user42
24-
; store notrap aligned region0 v2, v0
2524
; v4 = iadd v1, v1
2625
; return v4
2726
; }
27+

‎cranelift/filetests/filetests/alias/dead-store-then-idempotent-store.clif‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
test optimize precise-output
2+
set opt_level=speed
3+
target x86_64
4+
5+
;; Removing a dead store must expose the *previous* memory version to the store
6+
;; that overwrote it, so that a save/clear/restore sequence collapses entirely in
7+
;; a single pass rather than one link per pass.
8+
function %save_clear_restore(i64) {
9+
region0 = 0 "flags"
10+
block0(v0: i64):
11+
v1 = load.i32 notrap aligned region0 v0
12+
v2 = iconst.i32 0
13+
store notrap aligned region0 v2, v0
14+
store notrap aligned region0 v1, v0
15+
return
16+
}
17+
18+
; function %save_clear_restore(i64) fast {
19+
; region0 = 0 "flags"
20+
;
21+
; block0(v0: i64):
22+
; v1 = load.i32 notrap aligned region0 v0
23+
; return
24+
; }
25+
26+
;; The same, but with several dead stores between the load and the restore.
27+
function %save_clobber_many_restore(i64, i32, i32) {
28+
region0 = 0 "flags"
29+
block0(v0: i64, v1: i32, v2: i32):
30+
v3 = load.i32 notrap aligned region0 v0
31+
store notrap aligned region0 v1, v0
32+
store notrap aligned region0 v2, v0
33+
store notrap aligned region0 v1, v0
34+
store notrap aligned region0 v3, v0
35+
return
36+
}
37+
38+
; function %save_clobber_many_restore(i64, i32, i32) fast {
39+
; region0 = 0 "flags"
40+
;
41+
; block0(v0: i64, v1: i32, v2: i32):
42+
; v3 = load.i32 notrap aligned region0 v0
43+
; return
44+
; }
45+
46+
;; Two independent flags, each in its own alias region, are both collapsed.
47+
;;
48+
;; Note that the accesses are interleaved: unwinding one region's dead store
49+
;; must not disturb the other region's last-store state.
50+
function %two_regions_interleaved(i64, i64) {
51+
region0 = 0 "flags0"
52+
region1 = 1 "flags1"
53+
block0(v0: i64, v1: i64):
54+
v2 = load.i32 notrap aligned region0 v0
55+
v3 = load.i32 notrap aligned region1 v1
56+
v4 = iconst.i32 0
57+
store notrap aligned region0 v4, v0
58+
store notrap aligned region1 v4, v1
59+
store notrap aligned region0 v2, v0
60+
store notrap aligned region1 v3, v1
61+
return
62+
}
63+
64+
; function %two_regions_interleaved(i64, i64) fast {
65+
; region0 = 0 "flags0"
66+
; region1 = 1 "flags1"
67+
;
68+
; block0(v0: i64, v1: i64):
69+
; v2 = load.i32 notrap aligned region0 v0
70+
; v3 = load.i32 notrap aligned region1 v1
71+
; return
72+
; }
73+
74+
;; The restore is folded across intervening blocks, so long as nothing in them
75+
;; observes the flag.
76+
function %save_clear_restore_cross_block(i64) {
77+
region0 = 0 "flags"
78+
block0(v0: i64):
79+
v1 = load.i32 notrap aligned region0 v0
80+
v2 = iconst.i32 0
81+
store notrap aligned region0 v2, v0
82+
jump block1
83+
84+
block1:
85+
jump block2
86+
87+
block2:
88+
store notrap aligned region0 v1, v0
89+
return
90+
}
91+
92+
; function %save_clear_restore_cross_block(i64) fast {
93+
; region0 = 0 "flags"
94+
;
95+
; block0(v0: i64):
96+
; v1 = load.i32 notrap aligned region0 v0
97+
; jump block1
98+
;
99+
; block1:
100+
; jump block2
101+
;
102+
; block2:
103+
; return
104+
; }
105+
106+
;; Negative test: a call between the clear and the restore observes the cleared
107+
;; flag, so neither store may be removed.
108+
function %call_observes_cleared_flag(i64) {
109+
region0 = 0 "flags"
110+
fn0 = %g(i64)
111+
block0(v0: i64):
112+
v1 = load.i32 notrap aligned region0 v0
113+
v2 = iconst.i32 0
114+
store notrap aligned region0 v2, v0
115+
call fn0(v0)
116+
store notrap aligned region0 v1, v0
117+
return
118+
}
119+
120+
; function %call_observes_cleared_flag(i64) fast {
121+
; region0 = 0 "flags"
122+
; sig0 = (i64) fast
123+
; fn0 = %g sig0
124+
;
125+
; block0(v0: i64):
126+
; v1 = load.i32 notrap aligned region0 v0
127+
; v2 = iconst.i32 0
128+
; store notrap aligned region0 v2, v0 ; v2 = 0
129+
; call fn0(v0)
130+
; store notrap aligned region0 v1, v0
131+
; return
132+
; }
133+
134+
;; Negative test: the final store writes a value other than the saved one, so it
135+
;; is not idempotent. Only the dead middle store is removed.
136+
function %restore_wrong_value(i64, i32) {
137+
region0 = 0 "flags"
138+
block0(v0: i64, v1: i32):
139+
v2 = load.i32 notrap aligned region0 v0
140+
v3 = iconst.i32 0
141+
store notrap aligned region0 v3, v0
142+
store notrap aligned region0 v1, v0
143+
return
144+
}
145+
146+
; function %restore_wrong_value(i64, i32) fast {
147+
; region0 = 0 "flags"
148+
;
149+
; block0(v0: i64, v1: i32):
150+
; v2 = load.i32 notrap aligned region0 v0
151+
; store notrap aligned region0 v1, v0
152+
; return
153+
; }
154+
155+
;; Negative test: rolling back to the previous memory version must not resurrect
156+
;; knowledge across a store to a *different* address in the same region. The
157+
;; region's last-store slot is per-region, not per-address, so after the store to
158+
;; `v0+8` the analysis no longer knows what is at `v0`, and the final store to
159+
;; `v0` cannot be proven idempotent.
160+
function %same_region_different_address(i64, i32) {
161+
region0 = 0 "flags"
162+
block0(v0: i64, v1: i32):
163+
v2 = load.i32 notrap aligned region0 v0
164+
v3 = iconst.i32 0
165+
store notrap aligned region0 v3, v0
166+
store notrap aligned region0 v1, v0+8
167+
store notrap aligned region0 v2, v0
168+
return
169+
}
170+
171+
; function %same_region_different_address(i64, i32) fast {
172+
; region0 = 0 "flags"
173+
;
174+
; block0(v0: i64, v1: i32):
175+
; v2 = load.i32 notrap aligned region0 v0
176+
; v3 = iconst.i32 0
177+
; store notrap aligned region0 v3, v0 ; v3 = 0
178+
; store notrap aligned region0 v1, v0+8
179+
; store notrap aligned region0 v2, v0
180+
; return
181+
; }

‎tests/disas/component-model/direct-adapter-calls-inlining.wat‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@
104104
;; block9:
105105
;; v11 = load.i64 notrap aligned readonly can_move region3 v3+112
106106
;; v12 = load.i32 notrap aligned region4 v11
107-
;; store notrap aligned region4 v12, v11
108107
;; jump block13
109108
;;
110109
;; block13:

0 commit comments

Comments
 (0)