Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bulk array creation instructions #278

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Add bulk array creation instructions #278

merged 3 commits into from
Mar 9, 2022

Conversation

rossberg
Copy link
Member

@rossberg rossberg commented Feb 18, 2022

Addresses #260, following the V8 prototype spec, by adding:

  • array.new_data
  • array.new_elem
  • array.new_fixed

Also adds additional bulk operators to Post-MVP doc.

Instruction names differ from V8 doc to clarify where allocation happens and for consistency with memory and table instructions.

Moreover, make all *.new instructions constant, except array.new_elem.

Comment on lines 763 to 764
| 0xfb1d | `array.new_data $t $d` | `$t : typeidx`, `$d : dataidx` |
| 0xfb1e | `array.new_elem $t $e` | `$t : typeidx`, `$e : elemidx` |
Copy link
Member

Choose a reason for hiding this comment

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

@manoskouk, does the current array.init_from_data instruction in V8 work for both data and element segments or just for data segments?

Having two separate instructions for this seems reasonable and I like the new names, but could we choose a different opcode for array.new_elem? The problem with the current one is that it overlaps with V8's array.init_from_data_static, so implementing the new instruction with the opcode here will require a binary format back compat break in V8.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current array.new_from_data in v8 supports only numeric arrays and data segments. Regarding array.new_from_elem, the discussion on #260 did not come up with a decision regarding how to handle cyclic dependencies between the element section and constant uses of array.new_from_elem, so we postponed its implementation. We could, of course, introduce it as a non-constant-only expression for now.

Copy link
Member Author

@rossberg rossberg Feb 20, 2022

Choose a reason for hiding this comment

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

@tlively, ah, sorry, I had missed the difference in operands. Changed opcodes to 0x1b and 0x1c, which seem to be free right now.

@manoskouk, thanks for the reminder. So far, the MVP doc had not declared any of the allocation instructions constant, although we discussed it. I suppose there is agreement to change this, so I went ahead and incorporated that into this PR. And I agree that, for the time being, excluding array.new_elem from being constant is the simplest interim solution. Once we have introduced repeated sections, we can relax that.

Choose a reason for hiding this comment

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

FWIW, having these instructions as constant is essential for the dart2wasm use case of using them for constants. Including array.new_elem is necessary to not have a limit on how big List constants can be.

Copy link
Member Author

Choose a reason for hiding this comment

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

@askeksa-google, I see. Can't you use array.new_fixed as a workaround until we are able to relax the ordering of sections?

Choose a reason for hiding this comment

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

array.new_fixed works fine as long as the created array does not exceed the maximum number of operands for array.new_fixed. For longer arrays, array.new_elem is currently the only way to create them as constant expressions.

- and `$d` has mutable element type
- and `$s <: $d` modulo mutability
- the remaining operands are destination and source offset and length of the range
- `array.copy $d $s : [(ref null $d) i32 (ref null $s) i32 i32] -> []`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move array.copy to the MVP. J2CL has found it very useful in improving performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't be opposed to that, but it's a bit harder to argue that it is "MVP" material, given that it can be compiled out easily. Therefore, I would keep that decision separate from this PR, which focusses on an actual new ability, namely ways to create (useful) immutable arrays.

(Also, I'm not very keen on writing the test cases for such an instruction, so somebody else would have to own it as a PR. ;) )

@rossberg rossberg changed the title Add bulk array instructions Add bulk array creation instructions Feb 24, 2022
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Separately moving array.copy to the MPV sounds fine to me.

@rossberg rossberg merged commit 86f5724 into main Mar 9, 2022
@rossberg rossberg deleted the array.bulk branch March 9, 2022 10:29
@rossberg
Copy link
Member Author

rossberg commented Mar 9, 2022

Actually, allowing array.new_data in global initialisers is a problem as well, since it would be a forward reference to the data section, which in turn may reference globals. Even though this mutual recursion is easier to disentangle in an implementation than the situation for array.new_elem, making this exception would create havoc with with the envisioned generalisation to repeated but ordered sections. So I'll revert making it a constant instruction as well for the time being.

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.

4 participants