Skip to content

Commit 89b0ce0

Browse files
fitzgenfitzgen
committed
address review feedback and make needs-stack-maps decision more precise
1 parent 3af1911 commit 89b0ce0

File tree

4 files changed

+120
-18
lines changed

4 files changed

+120
-18
lines changed

‎crates/cranelift/src/func_environ.rs‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,6 +1371,17 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
13711371
builder.ins().stack_store(vmctx, slot, 0);
13721372
}
13731373
}
1374+
1375+
pub(crate) fn val_ty_needs_stack_map(&self, ty: WasmValType) -> bool {
1376+
match ty {
1377+
WasmValType::Ref(r) => self.heap_ty_needs_stack_map(r.heap_type),
1378+
_ => false,
1379+
}
1380+
}
1381+
1382+
pub(crate) fn heap_ty_needs_stack_map(&self, ty: WasmHeapType) -> bool {
1383+
ty.is_vmgcref_type_and_not_i31() && !ty.is_bottom()
1384+
}
13741385
}
13751386

13761387
impl TranslateTrap for FuncEnvironment<'_> {
@@ -2552,13 +2563,7 @@ impl<'module_environment> TargetEnvironment for FuncEnvironment<'module_environm
25522563

25532564
fn reference_type(&self, wasm_ty: WasmHeapType) -> (ir::Type, bool) {
25542565
let ty = crate::reference_type(wasm_ty, self.pointer_type());
2555-
let needs_stack_map = match wasm_ty.top() {
2556-
WasmHeapTopType::Extern | WasmHeapTopType::Any | WasmHeapTopType::Exn => true,
2557-
WasmHeapTopType::Func => false,
2558-
// TODO(#10248) Once continuations can be stored on the GC heap, we
2559-
// will need stack maps for continuation objects.
2560-
WasmHeapTopType::Cont => false,
2561-
};
2566+
let needs_stack_map = self.heap_ty_needs_stack_map(wasm_ty);
25622567
(ty, needs_stack_map)
25632568
}
25642569

‎crates/cranelift/src/func_environ/gc/enabled/drc.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -585,8 +585,7 @@ impl GcCompiler for DrcCompiler {
585585

586586
assert!(ty.is_vmgcref_type());
587587

588-
let (reference_type, needs_stack_map) = func_env.reference_type(ty.heap_type);
589-
debug_assert!(needs_stack_map);
588+
let (reference_type, _) = func_env.reference_type(ty.heap_type);
590589

591590
// Special case for references to uninhabited bottom types: the
592591
// reference must either be nullable and we can just eagerly return
@@ -737,8 +736,7 @@ impl GcCompiler for DrcCompiler {
737736
) -> WasmResult<()> {
738737
assert!(ty.is_vmgcref_type());
739738

740-
let (ref_ty, needs_stack_map) = func_env.reference_type(ty.heap_type);
741-
debug_assert!(needs_stack_map);
739+
let (ref_ty, _) = func_env.reference_type(ty.heap_type);
742740

743741
// Special case for references to uninhabited bottom types: either the
744742
// reference is nullable and we can just eagerly store null into `dst`

‎crates/cranelift/src/translate/code_translator.rs‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -224,13 +224,22 @@ pub fn translate_operator(
224224

225225
let val = builder.ins().select(cond, arg1, arg2);
226226

227-
if operand_types[..2].iter().any(|ty| match ty {
228-
WasmValType::Ref(r) => {
229-
let (_, needs_stack_map) = environ.reference_type(r.heap_type);
230-
needs_stack_map
231-
}
232-
_ => false,
233-
}) {
227+
// If either of the input types need inclusion in stack maps, then
228+
// the result will as well.
229+
//
230+
// Note that we don't need to check whether the result's type needs
231+
// inclusion in stack maps (that would be a conservative over
232+
// approximation) because the input types give us more-precise
233+
// information than the result type does. For example, the result
234+
// does not need inclusion in stack maps in the scenario where both
235+
// inputs are `i31ref`s and the result is an `anyref`. Even though
236+
// `anyref`s normally do require inclusion in stack maps, in this
237+
// particular case, we know that we are dealing with an `anyref`
238+
// that doesn't actually require inclusion.
239+
if operand_types
240+
.iter()
241+
.any(|ty| environ.val_ty_needs_stack_map(*ty))
242+
{
234243
builder.declare_value_needs_stack_map(val);
235244
}
236245

‎tests/disas/gc/typed-select-and-stack-maps.wat‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
;;! target = "x86_64"
2+
;;! flags = "-W function-references,gc"
3+
;;! test = "optimize"
4+
5+
(module
6+
(import "" "observe" (func $observe (param anyref)))
7+
(import "" "safepoint" (func $safepoint))
8+
9+
(func (param structref i31ref i32)
10+
;; Select from two types, one of which requires inclusion in stack maps,
11+
;; resulting in a value that also requires inclusion in stack maps.
12+
(select (result anyref)
13+
(local.get 0)
14+
(local.get 1)
15+
(local.get 2))
16+
17+
;; Make a call, which is a safepoint and has stack maps.
18+
(call $safepoint)
19+
20+
;; Observe the result of the select to keep it alive across the call.
21+
call $observe
22+
)
23+
24+
(func (param i31ref i31ref i32)
25+
;; Select from two types that do not require inclusion in stack maps,
26+
;; resulting in one that (normally) does. In this case, however, we
27+
;; shouldn't include the value in a stack map, because we know that the
28+
;; anyref cannot be an instance of a subtype that actually does require
29+
;; inclusion in stack maps.
30+
(select (result anyref)
31+
(local.get 0)
32+
(local.get 1)
33+
(local.get 2))
34+
35+
;; Make a call, which is a safepoint and has stack maps.
36+
(call $safepoint)
37+
38+
;; Observe the result of the select to keep it alive across the call.
39+
call $observe
40+
)
41+
)
42+
;; function u0:0(i64 vmctx, i64, i32, i32, i32) tail {
43+
;; ss0 = explicit_slot 4, align = 4
44+
;; gv0 = vmctx
45+
;; gv1 = load.i64 notrap aligned readonly gv0+8
46+
;; gv2 = load.i64 notrap aligned gv1+24
47+
;; gv3 = vmctx
48+
;; sig0 = (i64 vmctx, i64) tail
49+
;; sig1 = (i64 vmctx, i64, i32) tail
50+
;; stack_limit = gv2
51+
;;
52+
;; block0(v0: i64, v1: i64, v2: i32, v3: i32, v4: i32):
53+
;; @0049 v5 = select v4, v2, v3
54+
;; v14 = stack_addr.i64 ss0
55+
;; store notrap v5, v14
56+
;; @004c v8 = load.i64 notrap aligned readonly can_move v0+72
57+
;; @004c v7 = load.i64 notrap aligned readonly can_move v0+88
58+
;; @004c call_indirect sig0, v8(v7, v0), stack_map=[i32 @ ss0+0]
59+
;; v12 = load.i32 notrap v14
60+
;; @004e v11 = load.i64 notrap aligned readonly can_move v0+48
61+
;; @004e v10 = load.i64 notrap aligned readonly can_move v0+64
62+
;; @004e call_indirect sig1, v11(v10, v0, v12)
63+
;; @0050 jump block1
64+
;;
65+
;; block1:
66+
;; @0050 return
67+
;; }
68+
;;
69+
;; function u0:1(i64 vmctx, i64, i32, i32, i32) tail {
70+
;; gv0 = vmctx
71+
;; gv1 = load.i64 notrap aligned readonly gv0+8
72+
;; gv2 = load.i64 notrap aligned gv1+24
73+
;; gv3 = vmctx
74+
;; sig0 = (i64 vmctx, i64) tail
75+
;; sig1 = (i64 vmctx, i64, i32) tail
76+
;; stack_limit = gv2
77+
;;
78+
;; block0(v0: i64, v1: i64, v2: i32, v3: i32, v4: i32):
79+
;; @005c v8 = load.i64 notrap aligned readonly can_move v0+72
80+
;; @005c v7 = load.i64 notrap aligned readonly can_move v0+88
81+
;; @005c call_indirect sig0, v8(v7, v0)
82+
;; @005e v11 = load.i64 notrap aligned readonly can_move v0+48
83+
;; @005e v10 = load.i64 notrap aligned readonly can_move v0+64
84+
;; @0059 v5 = select v4, v2, v3
85+
;; @005e call_indirect sig1, v11(v10, v0, v5)
86+
;; @0060 jump block1
87+
;;
88+
;; block1:
89+
;; @0060 return
90+
;; }

0 commit comments

Comments
 (0)