Stream: git-wasmtime

Topic: wasmtime / PR #3101 Implement JumpTable branches in Crane...

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:12):

afonso360 opened PR #3101 from interpreter-more-branches to main:

:wave:

This PR Implements branches that use jump tables in the interpreter. Some additional questions inline.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:17):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:17):

afonso360 created PR review comment:

Both of these tests don't work in the actual compiler, but I'm not sure if its a bug.

It looks like we convert from br_tables into indirect_jump_table_br at some point during compilation. But do we support these instructions directly?

The crash that i'm seeing is the following:

thread 'worker #0' panicked at 'assertion failed: self.f.dfg[inst].opcode() == Opcode::BrTable', cranelift\codegen\src\machinst\lower.rs:901:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
FAIL .\filetests\filetests\runtests\br_jumptable.clif: panicked in worker #0: assertion failed: self.f.dfg[inst].opcode() == Opcode::BrTable
1 tests
Error: 1 failure

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:20):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:20):

afonso360 created PR review comment:

There are more arguments to this instruction, namely size and the jump table.

• Should we perform any sort of assertion on size?

• If the user tries to request an entry out of bounds, do we allow them to do that and then trap in IndirectJumpTableBr ? Or do we prevent that entry from being generated here?

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:21):

afonso360 created PR review comment:

I just noticed that this is probably not the most applicable trap code (call instead of jump?), should I create a new one?

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:21):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:32):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:32):

bjorn3 created PR review comment:

These instructions are only used by the old x86 backend as part of a lowering from br_table. The new x64 backend uses br_table directly.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 17:47):

afonso360 created PR review comment:

Oh, I didn't realize that, I thought that they were still valid because this test which targets aarch64 still uses them.

I'll remove the implementation for this from the PR

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:00):

afonso360 updated PR #3101 from interpreter-more-branches to main.

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:07):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:07):

afonso360 created PR review comment:

By the way, are brif and brff also replaced by br_icmp/fcmp?

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:33):

afonso360 updated PR #3101 from interpreter-more-branches to main.

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:33):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 18:33):

bjorn3 created PR review comment:

I don't think that is decided yet.

Wasmtime GitHub notifications bot (Jul 20 2021 at 22:31):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2021 at 22:31):

abrown merged PR #3101.

Last updated: Dec 23 2024 at 12:05 UTC