Skip to content

Commit c267eed

Browse files
alexcrichtonm0g3rgrandpigeduardomourarfitzgen
authored
[48.0.0] Some more backports (#14138)
* Fix panic compiling an empty component with debug info (#14130) generate_simulated_dwarf unwrapped the first core-module translation to name its compilation unit, but a component with no core modules has no translations, so `wasmtime compile -D debug-info=y` panicked on a valid `(component)` input. Return early instead: with no translations there are no functions to describe. * Run linker callback finalizers on invalid names (#14126) * Shuffle more finalizers in the C API (#14133) * Shuffle more finalizers in the C API This implements a similar refactoring to #14126 but for the component linker as well. * Clang-format * fix(wasmtime-cli): generic eio error thrown for wasip2 (#14107) * 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. * Alias analysis: do not restore the last-fence into a region slot (#14134) * Alias analysis: do not restore the last-fence into a region slot When we eliminate a dead store, we undo the effects that the dead store had on the `LastStore` state. However, querying the last store for a particular region falls back to the last fence, and we were incorrectly restoring that last fence into the region slot, rather than resetting the region slot to `None`. While technically incorrect, it was generally benign, but it did lead to "observing" instructions that we didn't mark observed during our initial observation pass, which ultimately led to debug assertion failures. Fixes #14131 * untrim whitespace in filetests * Return is-directory when a directory fd is used as a file (#14135) * wasip2: return is-directory when a directory fd is used as a file Descriptor::file() treated a directory as a bad descriptor. POSIX read/write on a directory is EISDIR, and wasi:filesystem already has is-directory. Preview1 guests still get EBADF (separate match and adapter). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> * wasip2: map is-directory only on read-via-stream Descriptor::file() must stay bad-descriptor for directories. wasi-testsuite filesystem-advise expects that for advise. Return is-directory from read-via-stream only (p2 result, p3 result future) so a directory read matches POSIX EISDIR. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> --------- Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> * Fix test expectations --------- Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> Co-authored-by: m0g3r <87276771+m0g3r@users.noreply.github.com> Co-authored-by: grandpig <grandpig@outlook.com> Co-authored-by: Eduardo de Moura Rodrigues <16357187+eduardomourar@users.noreply.github.com> Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> Co-authored-by: Sebastien Tardif <SebTardif@ncf.ca>
1 parent fc78952 commit c267eed

28 files changed

Lines changed: 827 additions & 64 deletions

‎cranelift/codegen/src/alias_analysis.rs‎

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

427+
/// Get the contents of `inst`'s own alias region's slot, without falling
428+
/// back to the last fence.
429+
///
430+
/// Returns `None` when `inst` has no alias region.
431+
fn raw_region_slot(&self, func: &Function, inst: Inst) -> Option<PackedOption<Inst>> {
432+
let region = func.dfg.insts[inst].alias_region(&func.dfg)?;
433+
Some(self.regions[region])
434+
}
435+
436+
/// Roll this state back to the memory version from just before `dead`,
437+
/// which is a store being removed from the function by dead-store
438+
/// elimination.
439+
///
440+
/// `prev_region_slot` must be what `dead`'s own alias-region slot held
441+
/// immediately before `dead` overwrote it, as recorded by `region_slot`
442+
/// when `dead` itself was processed (that is, it must not be the last-fence
443+
/// fallback).
444+
///
445+
/// Only `dead`'s own alias-region slot is restored. A store with no alias
446+
/// region is treated as a fence by `update`, which clears *every* region
447+
/// slot, and we do not undo that; in that case, we leave this state
448+
/// alone. Similarly, stores marked observed while processing `dead` stay
449+
/// observed.
450+
fn undo_store(&mut self, func: &Function, dead: Inst, prev_region_slot: PackedOption<Inst>) {
451+
debug_assert!(func.dfg.insts[dead].opcode().can_store());
452+
453+
let Some(region) = func.dfg.insts[dead].alias_region(&func.dfg) else {
454+
return;
455+
};
456+
457+
// Only roll back if `dead` really is the current last store to its
458+
// region.
459+
if self.regions[region].expand() == Some(dead) {
460+
self.regions[region] = prev_region_slot;
461+
}
462+
}
463+
427464
/// Get the last-store instruction for the given `inst`'s alias region, if
428465
/// any.
429466
fn get_last_store(&self, func: &Function, inst: Inst) -> PackedOption<Inst> {
@@ -527,6 +564,31 @@ struct MemoryLoc {
527564
extending_opcode: Option<Opcode>,
528565
}
529566

567+
/// What is known to be in memory at an associated `MemoryLoc`.
568+
#[derive(Clone, Copy, Debug)]
569+
struct KnownValue {
570+
/// The value held at the associated `MemoryLoc`.
571+
value: Value,
572+
573+
/// The instruction that produced `value`: either the load that read it out
574+
/// of memory or the store that wrote it there.
575+
///
576+
/// Kept around for quick dominance checks.
577+
def_inst: Inst,
578+
579+
/// When this entry was created by a store to a particular alias region,
580+
/// whatever that region's last-store slot held just *before* `def_inst`
581+
/// overwrote it, as given by `LastStores::region_slot`.
582+
///
583+
/// `None` means either the entry was created by a load or by a store with
584+
/// no alias region. Neither will ever undo `LastStores` state.
585+
///
586+
/// `Some(maybe_inst)` contains the alias region slot's previous value, so
587+
/// that it can be restored if `def_inst` is a dead store that gets
588+
/// eliminated.
589+
prev_region_slot: Option<PackedOption<Inst>>,
590+
}
591+
530592
/// The result of processing an instruction through alias analysis.
531593
pub enum OptResult {
532594
/// No optimization applied.
@@ -576,9 +638,7 @@ pub struct AliasAnalysis<'a> {
576638
/// Known memory-value equivalences. This is the result of the
577639
/// analysis. This is a mapping from (last store, address
578640
/// 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)>,
641+
mem_values: FxHashMap<MemoryLoc, KnownValue>,
582642
}
583643

584644
impl<'a> AliasAnalysis<'a> {
@@ -754,7 +814,37 @@ impl<'a> AliasAnalysis<'a> {
754814
ty,
755815
extending_opcode: get_ext_opcode(opcode),
756816
};
757-
self.mem_values.remove(&dead_loc);
817+
let dead_entry = self.mem_values.remove(&dead_loc);
818+
819+
// Roll our last-store state back to the memory version
820+
// just before the dead store, so that `state` describes
821+
// memory as if the dead store had never happened.
822+
//
823+
// Our callers remove the dead store from the layout and
824+
// then reprocess this overwriting store. Without the
825+
// rollback, that reprocessing keys its `mem_values`
826+
// lookup on the instruction we just removed, finds
827+
// nothing, and so fails to notice that the overwriter
828+
// has now become an idempotent store. Chains like
829+
//
830+
// v1 = load.i32 region0 v0
831+
// store region0 v2, v0 ;; dead
832+
// store region0 v1, v0 ;; idempotent, once the
833+
// ;; dead store is gone
834+
//
835+
// would then need a whole additional pass over the
836+
// function to collapse each link.
837+
//
838+
// A missing entry means we have no previous version to
839+
// roll back to, and simply don't: either we never
840+
// processed the dead store as a store in this pass (it
841+
// can come from a precomputed `block_input` snapshot,
842+
// for a predecessor block we have not walked yet) or it
843+
// has no alias region and therefore no slot of its own.
844+
if let Some(prev) = dead_entry.and_then(|e| e.prev_region_slot) {
845+
state.undo_store(func, last_store, prev);
846+
}
847+
758848
return OptResult::DeadStore {
759849
dead: last_store,
760850
overwriter: inst,
@@ -769,7 +859,12 @@ impl<'a> AliasAnalysis<'a> {
769859
ty,
770860
extending_opcode: get_ext_opcode(opcode),
771861
};
772-
if let Some((def_inst, known_value)) = self.mem_values.get(&check_loc).cloned() {
862+
if let Some(KnownValue {
863+
def_inst,
864+
value: known_value,
865+
..
866+
}) = self.mem_values.get(&check_loc).cloned()
867+
{
773868
// Check for idempotent stores, where we are
774869
// storing the exact same value back to a location
775870
// that already has that value.
@@ -806,7 +901,18 @@ impl<'a> AliasAnalysis<'a> {
806901
extending_opcode: get_ext_opcode(opcode),
807902
};
808903
trace!(" --> updating known values in memory: {mem_loc:?} = {store_data}");
809-
self.mem_values.insert(mem_loc, (inst, store_data));
904+
self.mem_values.insert(
905+
mem_loc,
906+
KnownValue {
907+
def_inst: inst,
908+
value: store_data,
909+
// NB: we use the raw region slot, without the
910+
// last-fence fallback, because we don't want to move an
911+
// instruction without a region into a region slot on
912+
// DSE rollback.
913+
prev_region_slot: state.raw_region_slot(func, inst),
914+
},
915+
);
810916

811917
OptResult::None
812918
} else if opcode.can_load() {
@@ -831,8 +937,9 @@ impl<'a> AliasAnalysis<'a> {
831937
// load (stores will always dominate though if
832938
// their `last_store` survives through
833939
// meet-points to this use-site).
834-
let aliased = if let Some((def_inst, value)) =
835-
self.mem_values.get(&mem_loc).cloned()
940+
let aliased = if let Some(KnownValue {
941+
def_inst, value, ..
942+
}) = self.mem_values.get(&mem_loc).cloned()
836943
{
837944
trace!(" see known value {value} from {def_inst}");
838945
if self.domtree.dominates(def_inst, inst, &func.layout) {
@@ -851,7 +958,16 @@ impl<'a> AliasAnalysis<'a> {
851958
// as a new equivalent value.
852959
if aliased.is_none() {
853960
trace!(" --> inserting load result {load_result} at loc {mem_loc:?}");
854-
self.mem_values.insert(mem_loc, (inst, load_result));
961+
self.mem_values.insert(
962+
mem_loc,
963+
KnownValue {
964+
def_inst: inst,
965+
value: load_result,
966+
// A load does not advance the memory version, so
967+
// there is no previous version to roll back to.
968+
prev_region_slot: None,
969+
},
970+
);
855971
}
856972

857973
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+
; }

0 commit comments

Comments
 (0)