Stream: git-wasmtime

Topic: wasmtime / PR #8134 cranelift: Choose memory trap code ba...

Wasmtime GitHub notifications bot (Mar 14 2024 at 16:58):

jameysharp requested abrown for a review on PR #8134.

Wasmtime GitHub notifications bot (Mar 14 2024 at 16:58):

jameysharp opened PR #8134 from jameysharp:memflags-trapcode to bytecodealliance:main:

Ideally we'd allow the frontend to specify what trap code a memory access instruction can produce, but we don't have room to store that information.

Instead, I'm adding additional meaning to the table and heap memflags. Besides being used in alias analysis, these flags now also indicate whether the instruction should produce a trap code of TableOutOfBounds or HeapOutOfBounds, respectively.

In the Cranelift weekly meeting we discussed that it would be nice to assert that either table or heap is set when the trap-code is requested, but I don't know if all users of the instruction encodings (such as cg-clif or Winch) set those flags, so I've chosen to default to the old behavior if none of the alias-analysis flags are set.

Currently, memory accesses with the table flag set always have notrap set as well, so the TableOutOfBounds case is never hit, but I wanted to separate this change out from the cranelift-wasm changes which will use it.

Wasmtime GitHub notifications bot (Mar 14 2024 at 16:58):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8134.

Wasmtime GitHub notifications bot (Mar 14 2024 at 17:32):

abrown commented on PR #8134:

@cfallin, @elliottt: do you want to review this instead? I am not as familiar with these trapping bits as I should be.

Wasmtime GitHub notifications bot (Mar 14 2024 at 17:40):

cfallin requested cfallin for a review on PR #8134.

Wasmtime GitHub notifications bot (Mar 14 2024 at 17:43):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2024 at 17:43):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2024 at 17:43):

cfallin created PR review comment:

I like the idea of this riding along with MemFlags -- it seems the most appropriate place to put it. However I'm not so sure about overloading the meaning of the alias-analysis bits; that feels like a future bug waiting to happen. Or at the very least, ties together a correctness-bearing concern with the particular implementation details of this version of an optimization pass. (E.g., what if we have a more detailed alias analysis in the future?)

We have a u16 for bitflags and only 9 flags defined currently; would you mind adding another two for table_trap and heap_trap?

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:16):

jameysharp updated PR #8134.

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:18):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:18):

jameysharp created PR review comment:

That concern makes sense, although I don't think we should define both table_trap and heap_trap as separate bits, because then what trap code should we return if neither is set? So instead I've introduced a single tabletrap bit, and if it's not set, the trap code is heap_oob as usual.

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:28):

cfallin created PR review comment:

OK, yep, that seems reasonable -- thanks!

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:28):

cfallin submitted PR review:

LGTM!

Wasmtime GitHub notifications bot (Mar 14 2024 at 18:28):

cfallin has enabled auto merge for PR #8134.

Wasmtime GitHub notifications bot (Mar 14 2024 at 19:07):

cfallin merged PR #8134.

Last updated: Nov 22 2024 at 16:03 UTC