Binary encoding + decoding + text printing support for i32.atomic.load ordering immediate

[stevenfontanella]

• Binary encoding + decoding for atomic ordering immediate for i32.atomic.load
  • The decoding is tested by the spec test. Encoding was checked manually by encoding the same spec test, as well as side-by-side with Binaryen's implementation, and by round-tripping the spec test (see below).
• Text printing for ordering immediate for i32.atomic.load
  • Tested by round-tripping the spec test (see below).

Binary format output:

xxd out.wasm
00000000: 0061 736d 0100 0000 0184 8080 8000 0160  .asm...........`
00000010: 0000 0382 8080 8000 0100 0584 8080 8000  ................
00000020: 0101 0101 0a9e 8080 8000 0198 8080 8000  ................
00000030: 0041 00fe 1002 001a 4100 fe10 1201 001a  .A......A.......
00000040: 4100 fe10 0200 1a0b                      A.......

Explainer for the binary format output:

1. • 0xfe10 - i32.atomic.load
   • 0x02 - alignment of 2 (2^2 = 4 bytes)
   • 0x00 - offset of 0
2. • 0xf10 - i32.atomic.load
   • 12 - alignment of 2 | bit 4 set indicating that an ordering immediate follows
   • 01 - acqrel ordering
   • 0x00 - offset of 0

The third case is encoded the same as the first, since SeqCst ordering is the default and we choose not to emit it in the binary format.

Round-tripped WAST shows that the text + binary are round-tripped correctly (seqcst immediate is dropped in both cases since it's the default):

./interpreter/wasm test/core/relaxed-atomics/relaxed-atomics.wast -o out.wast

(module
  (type $0 (func))
  (memory $0 1 1)
  (func $0
    (type 0)
    (i32.const 0)
    (i32.atomic.load)
    (drop)
    (i32.const 0)
    (i32.atomic.load acqrel)
    (drop)
    (i32.const 0)
    (i32.atomic.load)
    (drop)
  )
)
(assert_malformed
  (module quote
    "(memory 1 1 shared) (func $i32load (drop (i32.load acqrel (i32.const 51))))"
  )
  "unexpected token"
)
(module
  (type $0 (func))
  (memory $0 1 1)
  (func $0
    (type 0)
    (i32.const 0)
    (i32.atomic.load)
    (drop)
    (i32.const 0)
    (i32.atomic.load acqrel)
    (drop)
    (i32.const 0)
    (i32.atomic.load)
    (drop)
  )
)

[tlively]

Do notify and wait get atomicmemop just to make the types work out? They should never have memory order immediates.

[stevenfontanella]

Good point! Technically there would be no bug since we never emit SeqCst and that's the only possible ordering for these instructions. I agree that the code is misleading though. Changed memop to be polymorphic in the ordering field so that it can also be called with atomic instructions like this one, and changed atomicmemop to memop_with_ordering to make it clear which instructions emit an ordering and which ones don't.

[tlively]

I would still call this atomicmemop to match the decoder and the AST.

[stevenfontanella]

I think it's still fair to call e.g. memory.atomic.wait / memory.atomic.notify atomic even though they don't have explicit memory orderings. Maybe I'd rename the name in decode.ml to memop_with_ordering as well to match? The difference is whether we're parsing/emitting a memory ordering and not whether the instruction is atomic.

[tlively]

Instead of making this more polymorphic, maybe we should change the syntax so that atomic wait and notify take normal unordered memarg instead of atomicmemarg. WDYT?

[stevenfontanella]

Something like that is doable but it's mostly the naming that gets confusing. Currently notify is an atomicop which has an ordering. I wouldn't want to claim that atomic wait and notify are unordered or not atomicops, I think that isn't true, right? e.g. it wouldn't be correct to reorder memory.atomic.wait before or after any unordered memory operations.

I could do something like this:

type ordering = SeqCst | AcqRel
type unordered = Unordered
+ type seqcst = SeqCst'

type ('t, 'p, 'o) memop = {ty : 't; align : int; offset : int32; pack : 'p; ordering : 'o}
type loadop = (num_type, (pack_size * extension) option, unordered) memop
type storeop = (num_type, pack_size option, unordered) memop
type atomicop = (num_type, pack_size option, ordering) memop
+ type seqcstop = (num_type, pack_size option, seqcst) memop
...
+  | MemoryAtomicNotify of seqcstop    (* atomically notify all waiters at address *)

But then it's awkward because there are two different SeqCst constructors.

---

I could also change ordering, unordered, and seqcst into polymorphic variants (basically type-level sets of symbols, see this Gemini answer for context if you're curious), and then it would be easier to make type-level assertions about which orderings are possible, but I chose to avoid it until now just to avoid too much magic. (polymorphic variants have the downside that they are 'open' and can't protect against typos as well).

---

I don't have a great answer, so far this is what I lean towards even though it does make some invalid states possible at the type level.

[tlively]

Yeah, I can see it might be surprising that wait and notify get non-atomic memargs and their associated unordered ordering. But on the other hand this would be a better match for the concrete syntax, which disallows providing an order in their memargs. Maybe we should rename unordered to none to make it less confusing?

Either way, I don't think it's worth going out of the way to make sure wait and notify get seqcst orders in the syntax, as long as they are handled correctly in the semantics.