Skip to content

Commit b5b8f49

Browse files
fitzgenfitzgen
authored
Ensure that LastStores is a proper lattice (#14230)
There were two ways in which alias analysis's `LastStores` state was not a proper lattice, which made the order we processed the worklist and called `LastStores::meet` observable: 1. We didn't have a single, canonical bottom value for the last store to a region. We were taking the first instruction in a block as an identifier for control-flow join points so that we would get different `MemoryLoc`s for different control-flow joins, which is necessary to avoid illegally forwarding a value loaded inside one control-flow join to a load in another, different control-flow join. However, this meant that we effectively had multiple bottom elements, which made the path we descended through the "lattice" observable. The solution here is to make `LastStore::Unknown` a proper bottom for the lattice and then add an "extent token" to `MemoryLoc`. The extent is computed incrementally as we push and pop blocks from a pre-order traversal of the dominator tree (which the egraphs pass that drives alias analysis already performs). 2. We computed the observed-stores set while we computed the fixpoint of the initial `LastStores` inputs to each block. This was incorrect, however, because a `LastStores` could transiently contain a `LastStore::Inst` that disappears in later iterations of the fixpoint, and which instructions do or don't transiently appear in `LastStores` in that way depends on the order in which we call `LastStores::meet`. Therefore, observing stores while computing the fixpoint might or might not observe an instruction depending on the worklist processing order. The solution in this case is to only compute the observed-stores set after we've computed the `LastStores` fixpoint, at which point there are no transient `LastStore::Inst`s anymore.
1 parent 84dbb9f commit b5b8f49

7 files changed

Lines changed: 940 additions & 280 deletions

File tree

‎cranelift/codegen/src/alias_analysis.rs‎

Lines changed: 653 additions & 274 deletions
Large diffs are not rendered by default.

‎cranelift/codegen/src/egraph/mod.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
//! Support for egraphs represented in the DataFlowGraph.
22
33
use crate::FxHashSet;
4-
use crate::alias_analysis::{AliasAnalysis, LastStores, OptResult};
4+
use crate::alias_analysis::{AliasAnalysis, MemoryState, OptResult};
55
use crate::branch_to_trap::BranchToTrapAnalysis;
66
use crate::ctxhash::{CtxEq, CtxHash, NullCtx};
77
use crate::cursor::{Cursor, CursorPosition, FuncCursor};
@@ -155,7 +155,7 @@ where
155155
/// build a post-dominator tree for dead-store elimination.
156156
cfg: &'opt ControlFlowGraph,
157157
pub(crate) alias_analysis: &'opt mut AliasAnalysis<'analysis>,
158-
pub(crate) alias_analysis_state: &'opt mut LastStores,
158+
pub(crate) alias_analysis_state: &'opt mut MemoryState,
159159
pub(crate) branch_to_trap_analysis: &'opt mut BranchToTrapAnalysis,
160160
ctrl_plane: &'opt mut ControlPlane,
161161
// Held locally during optimization of one node (recursively):
@@ -923,11 +923,14 @@ impl<'a> EgraphPass<'a> {
923923
{
924924
gvn_map_blocks.pop();
925925
gvn_map.decrement_depth();
926+
self.alias_analysis.pop_scope();
926927
}
927928

928929
gvn_map.increment_depth();
929930
gvn_map_blocks.push(block);
930931

932+
let mut alias_analysis_state = self.alias_analysis.push_scope(&*self.cfg, block);
933+
931934
// Check that `gvn_map_blocks` is the path from this block up to the
932935
// root in the dominator tree.
933936
debug_assert_eq!(gvn_map_blocks, {
@@ -945,8 +948,6 @@ impl<'a> EgraphPass<'a> {
945948
trace!("Processing block {}", block);
946949
cursor.set_position(CursorPosition::Before(block));
947950

948-
let mut alias_analysis_state = self.alias_analysis.block_starting_state(block);
949-
950951
for &param in cursor.func.dfg.block_params(block) {
951952
trace!("creating initial singleton eclass for blockparam {}", param);
952953
value_to_opt_value[param] = param;

‎cranelift/filetests/filetests/alias/fence-fallback-across-join.clif‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
test optimize precise-output
2+
set opt_level=speed
3+
target aarch64
4+
5+
function %fence_fallback_survives_join(i64, i32) -> i32, i32 {
6+
region0 = 0 "R0"
7+
8+
block0(v0: i64, v1: i32):
9+
v3 = load.i32 notrap aligned region0 v0
10+
brif v1, block1, block2
11+
12+
block1:
13+
jump block3
14+
15+
block2:
16+
jump block3
17+
18+
block3:
19+
v4 = load.i32 notrap aligned region0 v0
20+
return v3, v4
21+
}
22+
23+
; function %fence_fallback_survives_join(i64, i32) -> i32, i32 fast {
24+
; region0 = 0 "R0"
25+
;
26+
; block0(v0: i64, v1: i32):
27+
; v3 = load.i32 notrap aligned region0 v0
28+
; brif v1, block1, block2
29+
;
30+
; block1:
31+
; jump block3
32+
;
33+
; block2:
34+
; jump block3
35+
;
36+
; block3:
37+
; return v3, v3
38+
; }
39+
40+
function %unknown_does_not_take_the_fence_fallback(i64, i32, i32) -> i32, i32 {
41+
region0 = 0 "R0"
42+
43+
block0(v0: i64, v1: i32, v2: i32):
44+
v3 = load.i32 notrap aligned region0 v0
45+
brif v1, block1, block2
46+
47+
block1:
48+
;; This store prevents `block3` from reusing `v3`.
49+
store notrap aligned region0 v2, v0
50+
jump block3
51+
52+
block2:
53+
jump block3
54+
55+
block3:
56+
v4 = load.i32 notrap aligned region0 v0
57+
return v3, v4
58+
}
59+
60+
; function %unknown_does_not_take_the_fence_fallback(i64, i32, i32) -> i32, i32 fast {
61+
; region0 = 0 "R0"
62+
;
63+
; block0(v0: i64, v1: i32, v2: i32):
64+
; v3 = load.i32 notrap aligned region0 v0
65+
; brif v1, block1, block2
66+
;
67+
; block1:
68+
; store.i32 notrap aligned region0 v2, v0
69+
; jump block3
70+
;
71+
; block2:
72+
; jump block3
73+
;
74+
; block3:
75+
; v4 = load.i32 notrap aligned region0 v0
76+
; return v3, v4
77+
; }

‎cranelift/filetests/filetests/alias/join-order-independence.clif‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
test optimize precise-output
2+
set opt_level=speed
3+
target aarch64
4+
5+
;; The last-store analysis must not depend on the order in which the worklist
6+
;; happens to visit blocks. These two functions differ only in the order of the
7+
;; `brif`'s two arms, so they must optimize identically.
8+
9+
function %arms_in_order(i64, i32, i32) -> i32, i32 {
10+
region0 = 0 "R0"
11+
12+
block0(v0: i64, v1: i32, v2: i32):
13+
store notrap aligned region0 v1, v0
14+
brif v1, block3, block1
15+
16+
block1:
17+
store notrap aligned region0 v2, v0
18+
jump block3
19+
20+
block3:
21+
v3 = load.i32 notrap aligned region0 v0
22+
jump block4
23+
24+
block4:
25+
v4 = load.i32 notrap aligned region0 v0
26+
return v3, v4
27+
}
28+
29+
; function %arms_in_order(i64, i32, i32) -> i32, i32 fast {
30+
; region0 = 0 "R0"
31+
;
32+
; block0(v0: i64, v1: i32, v2: i32):
33+
; store notrap aligned region0 v1, v0
34+
; brif v1, block3, block1
35+
;
36+
; block1:
37+
; store.i32 notrap aligned region0 v2, v0
38+
; jump block3
39+
;
40+
; block3:
41+
; v3 = load.i32 notrap aligned region0 v0
42+
; jump block4
43+
;
44+
; block4:
45+
; return v3, v3
46+
; }
47+
48+
function %arms_swapped(i64, i32, i32) -> i32, i32 {
49+
region0 = 0 "R0"
50+
51+
block0(v0: i64, v1: i32, v2: i32):
52+
store notrap aligned region0 v1, v0
53+
brif v1, block1, block3
54+
55+
block1:
56+
store notrap aligned region0 v2, v0
57+
jump block3
58+
59+
block3:
60+
v3 = load.i32 notrap aligned region0 v0
61+
jump block4
62+
63+
block4:
64+
v4 = load.i32 notrap aligned region0 v0
65+
return v3, v4
66+
}
67+
68+
; function %arms_swapped(i64, i32, i32) -> i32, i32 fast {
69+
; region0 = 0 "R0"
70+
;
71+
; block0(v0: i64, v1: i32, v2: i32):
72+
; store notrap aligned region0 v1, v0
73+
; brif v1, block1, block3
74+
;
75+
; block1:
76+
; store.i32 notrap aligned region0 v2, v0
77+
; jump block3
78+
;
79+
; block3:
80+
; v3 = load.i32 notrap aligned region0 v0
81+
; jump block4
82+
;
83+
; block4:
84+
; return v3, v3
85+
; }

‎cranelift/filetests/filetests/alias/join-single-predecessor.clif‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
test optimize precise-output
2+
set opt_level=speed
3+
target aarch64
4+
5+
function %single_pred_inherits(i64, i32, i32, i32) -> i32, i32 {
6+
region0 = 0 "R0"
7+
8+
block0(v0: i64, v1: i32, v2: i32, v3: i32):
9+
store notrap aligned region0 v1, v0
10+
jump block1
11+
12+
block1:
13+
brif v2, block2, block3
14+
15+
block3:
16+
jump block2
17+
18+
block2:
19+
v10 = load.i32 notrap aligned region0 v0
20+
jump block4
21+
22+
block4:
23+
;; `block4` has exactly one predecessor, `block2`, so at the fixpoint its
24+
;; region0 slot must equal `block2`'s outgoing value, and `v11` must
25+
;; therefore be forwarded to `v10`.
26+
v11 = load.i32 notrap aligned region0 v0
27+
store notrap aligned region0 v3, v0
28+
brif v1, block1, block5
29+
30+
block5:
31+
return v10, v11
32+
}
33+
34+
; function %single_pred_inherits(i64, i32, i32, i32) -> i32, i32 fast {
35+
; region0 = 0 "R0"
36+
;
37+
; block0(v0: i64, v1: i32, v2: i32, v3: i32):
38+
; store notrap aligned region0 v1, v0
39+
; jump block1
40+
;
41+
; block1:
42+
; brif.i32 v2, block2, block3
43+
;
44+
; block3:
45+
; jump block2
46+
;
47+
; block2:
48+
; v10 = load.i32 notrap aligned region0 v0
49+
; jump block4
50+
;
51+
; block4:
52+
; store.i32 notrap aligned region0 v3, v0
53+
; brif.i32 v1, block1, block5
54+
;
55+
; block5:
56+
; return v10, v10
57+
; }

‎cranelift/filetests/filetests/alias/merge-token-identity.clif‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
test optimize precise-output
2+
set opt_level=speed
3+
target aarch64
4+
5+
;; `v3` and `v4` sit after two independent joins, with `block4`'s store in
6+
;; between.
7+
function %two_independent_joins(i64, i32, i32) -> i32, i32 {
8+
region0 = 0 "R0"
9+
10+
block0(v0: i64, v1: i32, v2: i32):
11+
brif v1, block1, block2
12+
13+
block1:
14+
store notrap aligned region0 v2, v0
15+
jump block3
16+
17+
block2:
18+
jump block3
19+
20+
block3:
21+
v3 = load.i32 notrap aligned region0 v0
22+
brif v1, block4, block5
23+
24+
block4:
25+
store notrap aligned region0 v1, v0
26+
jump block6
27+
28+
block5:
29+
jump block6
30+
31+
block6:
32+
v4 = load.i32 notrap aligned region0 v0
33+
return v3, v4
34+
}
35+
36+
; function %two_independent_joins(i64, i32, i32) -> i32, i32 fast {
37+
; region0 = 0 "R0"
38+
;
39+
; block0(v0: i64, v1: i32, v2: i32):
40+
; brif v1, block1, block2
41+
;
42+
; block1:
43+
; store.i32 notrap aligned region0 v2, v0
44+
; jump block3
45+
;
46+
; block2:
47+
; jump block3
48+
;
49+
; block3:
50+
; v3 = load.i32 notrap aligned region0 v0
51+
; brif.i32 v1, block4, block5
52+
;
53+
; block4:
54+
; store.i32 notrap aligned region0 v1, v0
55+
; jump block6
56+
;
57+
; block5:
58+
; jump block6
59+
;
60+
; block6:
61+
; v4 = load.i32 notrap aligned region0 v0
62+
; return v3, v4
63+
; }

‎tests/disas/gc/array-copy-with-fuel.wat‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,6 @@
9393
;; @002b brif.i32 v6, block4, block7
9494
;;
9595
;; block4:
96-
;; v143 = load.i32 notrap aligned region6 v162
97-
;; v145 = load.i32 notrap aligned region7 v163
9896
;; @002b v89 = icmp.i64 ult v45, v69
9997
;; @002b v94 = iadd.i64 v45, v170
10098
;; @002b v95 = iadd.i64 v69, v170

0 commit comments

Comments
 (0)