Skip to content

Conversation

@brendandahl
Copy link
Collaborator

Prototype implementation of the new multibyte array proposal that reuses the existing memory instructions.
The syntax for the new instructions is as follows:
<i32|i64|f32|f64>.store type=array

Some spec related TODOs:

  • Decide on new instructions vs existing memory instructions with memarg flag.
  • If using existing instructions:
    • What does the wat syntax look like? e.g. i32.store type=array x y or i32.store array x y or ???
    • Do we support immediate offsets? - Do we support i32.store8? (effectively the same as array.set on i8)
  • Do we support atomic instructions?

Some implementation TODOs:

  • Add feature flag.
  • Add v128.store.
  • Go through the various visitArrayStore functions and implement them or make sure the copied versions makes sense.
  • Add fuzzing support.
  • Make it so there's only one WasmBinaryReader::getMemarg
  • Add JS API

Prototype implementation of the new multibyte array proposal that reuses
the existing memory instructions.
The syntax for the new instructions is as follows:
  <i32|i64|f32|f64>.store type=array <array ref> <index> <value>

Some spec related TODOs:
  - Decide on new instructions vs existing memory instructions
    with memarg flag.
  - If using existing instructions:
      - What does the wat syntax look like?
        e.g. `i32.store type=array x y` or `i32.store array x y` or ???
      - Do we support immediate offsets?
      - Do we support i32.store8? (effectively the same as array.set on
        i8)
  - Do we support atomic instructions?

Some implementation TODOs:
  - Add v128.store.
  - Go through the various `visitArrayStore` functions and implement
    them or make sure the copied versions makes sense.
  - Add fuzzing support.
  - Make it so there's only one `WasmBinaryReader::getMemarg`
  - Add JS API
Comment on lines +1846 to +1847
// TODO is this needed?
MemoryOrder order = MemoryOrder::Unordered;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably in the long run, but not urgently. I would leave it out for now.

)

;; CHECK: (func $stores (type $1)
;; CHECK-NEXT: (i32.store8 type=array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text format is going to need a type index immediate to give the type of the array that is being accessed. Maybe we can just use the presence of that immediate to signal that we're accessing an array? So we might have i32.store8 $myArray instead of i32.store8 type=array. Obviously we can't settle that here, but we're definitely going to need the immediate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the type immediate if it's only ever going to be an (array (mut i8))? I guess it could be a memref in the future or?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an infinite set of (array (mut i8)) types, and I would expect these instructions to work with all of them. For example, $A, $B, $C, and $D are all different types here:

(type $A (sub final (array (mut i8))))
(type $B (sub (array (mut i8))))
(type $C (sub $B (array (mut i8))))
(rec (type $D (array (mut i8))) (type (struct)))

Given that there are several types that could be accepted, we need the type index to make sure we can look at an instruction with its immediates in isolation and say what its input and output types will be. (This ensures we maintain the principal types property, which is important for analyzing instruction sequences in isolation. For example, it is important for outlining.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is to accept just a single, well-known array type, e.g. (rec (type (sub final (array (mut i8))))).0, but that would be somewhat limiting for producers, would require extra work in Binaryen, and would be unprecedented for core Wasm instructions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started playing with i32.store8 $myArray, but if I'm following this correctly, I don't think there's a way to distinguish multimemory from array store e.g. i32.store8 1 could be a memory or type index.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, good point :/ I guess we do need to distinguish this somehow. For prototyping I guess we can do whatever, but it would also be good to open a issue on the proposal repo to see what folks think is best here.

case BinaryConsts::I32StoreMem8: {
auto [mem, align, offset] = getMemarg();
return builder.makeStore(1, offset, align, Type::i32, mem);
auto [mem, align, offset, backing] = getMemargWithBacking();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The binary format will need the type immediate as well, so this will have to look something like this:

Suggested change
auto [mem, align, offset, backing] = getMemargWithBacking();
auto [mem, align, offset, backing] = getMemargWithBacking();
if (backing == BackingType::Array) {
HeapType type = getIndexedHeapType();
...

At that point we might as well add a separate IRBuilder method for making array stores, which should help separate concerns more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants