Skip to content

Commit 3932e8f

Browse files
bjorn3bjorn3
authored
Some fixes for try_call (#10593)
* Fix cranelift-frontend handling of try_call * Implement eliminate_unreachable_code for exception tables * Ensure try_call is considered a memory fence * Don't error on try_call in the verifier if no TargetIsa is passed * Don't clobber all registers for try_call unless the tail call conv is used This way other consumers of Cranelift don't have to pay the cost of the way Wasmtime will implement unwinding on exceptions. * Allow SystemV call conv with try_call
1 parent 4221073 commit 3932e8f

12 files changed

Lines changed: 213 additions & 55 deletions

File tree

‎cranelift/codegen/src/inst_predicates.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ pub fn has_memory_fence_semantics(op: Opcode) -> bool {
148148
| Opcode::AtomicStore
149149
| Opcode::Fence
150150
| Opcode::Debugtrap => true,
151-
Opcode::Call | Opcode::CallIndirect => true,
151+
Opcode::Call | Opcode::CallIndirect | Opcode::TryCall | Opcode::TryCallIndirect => true,
152152
op if op.can_trap() => true,
153153
_ => false,
154154
}

‎cranelift/codegen/src/ir/exception_table.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,12 @@ impl ExceptionTableData {
155155
pub fn signature(&self) -> SigRef {
156156
self.sig
157157
}
158+
159+
/// Clears all entries in this exception table, but leaves the function signature.
160+
pub fn clear(&mut self) {
161+
self.tags.clear();
162+
self.targets.clear();
163+
}
158164
}
159165

160166
/// A wrapper for the context required to display a

‎cranelift/codegen/src/isa/aarch64/abi.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1127,7 +1127,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
11271127
fn get_regs_clobbered_by_call(call_conv: isa::CallConv, is_exception: bool) -> PRegSet {
11281128
match call_conv {
11291129
isa::CallConv::Winch => WINCH_CLOBBERS,
1130-
_ if is_exception => ALL_CLOBBERS,
1130+
isa::CallConv::Tail if is_exception => ALL_CLOBBERS,
11311131
_ => DEFAULT_AAPCS_CLOBBERS,
11321132
}
11331133
}

‎cranelift/codegen/src/isa/call_conv.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,15 @@ impl CallConv {
8484
/// Does this calling convention support exceptions?
8585
pub fn supports_exceptions(&self) -> bool {
8686
match self {
87-
CallConv::Tail => true,
87+
CallConv::Tail | CallConv::SystemV => true,
8888
_ => false,
8989
}
9090
}
9191

9292
/// What types do the exception payload value(s) have?
9393
pub fn exception_payload_types(&self, pointer_ty: Type) -> &[Type] {
9494
match self {
95-
CallConv::Tail => match pointer_ty {
95+
CallConv::Tail | CallConv::SystemV => match pointer_ty {
9696
types::I32 => &[types::I32, types::I32],
9797
types::I64 => &[types::I64, types::I64],
9898
_ => unreachable!(),

‎cranelift/codegen/src/isa/riscv64/abi.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -639,13 +639,12 @@ impl ABIMachineSpec for Riscv64MachineDeps {
639639
}
640640

641641
fn get_regs_clobbered_by_call(
642-
_call_conv_of_callee: isa::CallConv,
642+
call_conv_of_callee: isa::CallConv,
643643
is_exception: bool,
644644
) -> PRegSet {
645-
if is_exception {
646-
ALL_CLOBBERS
647-
} else {
648-
DEFAULT_CLOBBERS
645+
match call_conv_of_callee {
646+
isa::CallConv::Tail if is_exception => ALL_CLOBBERS,
647+
_ => DEFAULT_CLOBBERS,
649648
}
650649
}
651650

‎cranelift/codegen/src/isa/s390x/abi.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,7 @@ impl ABIMachineSpec for S390xMachineDeps {
907907
is_exception: bool,
908908
) -> PRegSet {
909909
match call_conv_of_callee {
910-
_ if is_exception => ALL_CLOBBERS,
910+
isa::CallConv::Tail if is_exception => ALL_CLOBBERS,
911911
isa::CallConv::Tail => TAIL_CLOBBERS,
912912
_ => SYSV_CLOBBERS,
913913
}

‎cranelift/codegen/src/isa/x64/abi.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -914,7 +914,7 @@ impl ABIMachineSpec for X64ABIMachineSpec {
914914
match call_conv_of_callee {
915915
CallConv::Winch => ALL_CLOBBERS,
916916
CallConv::WindowsFastcall => WINDOWS_CLOBBERS,
917-
_ if is_exception => ALL_CLOBBERS,
917+
CallConv::Tail if is_exception => ALL_CLOBBERS,
918918
_ => SYSV_CLOBBERS,
919919
}
920920
}

‎cranelift/codegen/src/unreachable_code.rs‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,20 @@ pub fn eliminate_unreachable_code(
2222
let _tt = timing::unreachable_code();
2323
let mut pos = FuncCursor::new(func);
2424
let mut used_tables = EntitySet::with_capacity(pos.func.stencil.dfg.jump_tables.len());
25+
let mut used_exception_tables =
26+
EntitySet::with_capacity(pos.func.stencil.dfg.exception_tables.len());
2527
while let Some(block) = pos.next_block() {
2628
if domtree.is_reachable(block) {
2729
let inst = pos.func.layout.last_inst(block).unwrap();
28-
if let ir::InstructionData::BranchTable { table, .. } = pos.func.dfg.insts[inst] {
29-
used_tables.insert(table);
30+
match pos.func.dfg.insts[inst] {
31+
ir::InstructionData::BranchTable { table, .. } => {
32+
used_tables.insert(table);
33+
}
34+
ir::InstructionData::TryCall { exception, .. }
35+
| ir::InstructionData::TryCallIndirect { exception, .. } => {
36+
used_exception_tables.insert(exception);
37+
}
38+
_ => (),
3039
}
3140
continue;
3241
}
@@ -55,4 +64,10 @@ pub fn eliminate_unreachable_code(
5564
jt_data.clear();
5665
}
5766
}
67+
68+
for (exception, exception_data) in func.stencil.dfg.exception_tables.iter_mut() {
69+
if !used_exception_tables.contains(exception) {
70+
exception_data.clear();
71+
}
72+
}
5873
}

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

Lines changed: 23 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1464,23 +1464,6 @@ impl<'a> Verifier<'a> {
14641464
Ok(())
14651465
}
14661466

1467-
fn pointer_type_or_error(&self, inst: Inst, errors: &mut VerifierErrors) -> Result<Type, ()> {
1468-
// Ensure we have an ISA so we know what the pointer size is.
1469-
if let Some(isa) = self.isa {
1470-
Ok(isa.pointer_type())
1471-
} else {
1472-
errors
1473-
.fatal((
1474-
inst,
1475-
self.context(inst),
1476-
format!("need an ISA to validate correct pointer type"),
1477-
))
1478-
// Will always return an `Err`, but the `Ok` type
1479-
// doesn't match, so map it.
1480-
.map(|_| Type::default())
1481-
}
1482-
}
1483-
14841467
fn typecheck_block_call(
14851468
&self,
14861469
inst: Inst,
@@ -1504,7 +1487,9 @@ impl<'a> Verifier<'a> {
15041487
));
15051488
}
15061489
for (arg, param) in args.zip(block_params.iter()) {
1507-
let arg_ty = self.block_call_arg_ty(arg, inst, target_type, errors)?;
1490+
let Some(arg_ty) = self.block_call_arg_ty(arg, inst, target_type, errors)? else {
1491+
continue;
1492+
};
15081493
let param_ty = self.func.dfg.value_type(*param);
15091494
if arg_ty != param_ty {
15101495
errors.nonfatal((
@@ -1523,9 +1508,9 @@ impl<'a> Verifier<'a> {
15231508
inst: Inst,
15241509
target_type: BlockCallTargetType,
15251510
errors: &mut VerifierErrors,
1526-
) -> Result<Type, ()> {
1511+
) -> Result<Option<Type>, ()> {
15271512
match arg {
1528-
BlockArg::Value(v) => Ok(self.func.dfg.value_type(v)),
1513+
BlockArg::Value(v) => Ok(Some(self.func.dfg.value_type(v))),
15291514
BlockArg::TryCallRet(_) | BlockArg::TryCallExn(_) => {
15301515
// Get the invoked signature.
15311516
let et = match self.func.dfg.insts[inst].exception_table() {
@@ -1548,7 +1533,7 @@ impl<'a> Verifier<'a> {
15481533
(BlockArg::TryCallRet(i), BlockCallTargetType::ExNormalRet)
15491534
if (i as usize) < sig.returns.len() =>
15501535
{
1551-
Ok(sig.returns[i as usize].value_type)
1536+
Ok(Some(sig.returns[i as usize].value_type))
15521537
}
15531538
(BlockArg::TryCallRet(_), BlockCallTargetType::ExNormalRet) => {
15541539
errors.fatal((
@@ -1567,20 +1552,24 @@ impl<'a> Verifier<'a> {
15671552
unreachable!()
15681553
}
15691554
(BlockArg::TryCallExn(i), BlockCallTargetType::Exception) => {
1570-
match sig
1571-
.call_conv
1572-
.exception_payload_types(self.pointer_type_or_error(inst, errors)?)
1573-
.get(i as usize)
1574-
{
1575-
Some(ty) => Ok(*ty),
1576-
None => {
1577-
errors.fatal((
1578-
inst,
1579-
self.context(inst),
1580-
format!("out-of-bounds `exnN` block argument"),
1581-
))?;
1582-
unreachable!()
1555+
if let Some(isa) = self.isa {
1556+
match sig
1557+
.call_conv
1558+
.exception_payload_types(isa.pointer_type())
1559+
.get(i as usize)
1560+
{
1561+
Some(ty) => Ok(Some(*ty)),
1562+
None => {
1563+
errors.fatal((
1564+
inst,
1565+
self.context(inst),
1566+
format!("out-of-bounds `exnN` block argument"),
1567+
))?;
1568+
unreachable!()
1569+
}
15831570
}
1571+
} else {
1572+
Ok(None)
15841573
}
15851574
}
15861575
(BlockArg::TryCallExn(_), _) => {

‎cranelift/filetests/filetests/egraph/try_call-mem-clobber.clif‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
; Regression test for the alias analysis not considering try_call to be a memory fence
2+
3+
test optimize
4+
set opt_level=speed_and_size
5+
target x86_64
6+
7+
function u0:0(i64 sret, i64) system_v {
8+
ss0 = explicit_slot 8
9+
sig0 = (i64) system_v
10+
fn0 = u0:1 sig0
11+
12+
block0(v0: i64, v1: i64):
13+
v20 = stack_addr.i64 ss0
14+
; check: v20 = stack_addr.i64 ss0
15+
store v1, v20 ; store v1 to ss0
16+
try_call fn0(v20), sig0, block1, []
17+
18+
block1:
19+
v21 = stack_addr.i64 ss0
20+
v2 = load.i64 v21; load v2 from ss0 after the fn0 call potentially changed it
21+
; check: v2 = load.i64 v20
22+
store v2, v0 ; v2 used to be incorrectly replaced by v1 in the egraph pass
23+
; nextln: store v2, v0
24+
return
25+
}

0 commit comments

Comments
 (0)