Skip to content
This repository was archived by the owner on Nov 3, 2021. It is now read-only.

Generalize test generators for table.copy and table.init to multi-table #84

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Generalize test generators for table.copy and table.init to multi-table #84

merged 2 commits into from
Mar 24, 2020

Conversation

lars-t-hansen
Copy link
Contributor

Fixes #74.

@lars-t-hansen
Copy link
Contributor Author

@rossberg / @binji, PTAL. I'm not allowed to suggest reviewers.

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

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

One question about element/table index order, but otherwise lgtm. (I also tested these with wabt and all the tests passed there too)


for ( let table of [0, 1] ) {
// Passive init that overwrites all-null entries
tab_test(`(table.init $t${table} 1 (i32.const 7) (i32.const 0) (i32.const 4))`,
Copy link
Member

Choose a reason for hiding this comment

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

We recently clarified in the binary format that elem index comes before table index. Seems like we'll want to do the same for the text format too? If so, we may want to do that before landing this change.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep this as an oddity of the binary format. For the human-readable text form (and the AST) it's nicer if init matches the dst-src parameter order of copy and the operands of other instructions.

Copy link
Member

Choose a reason for hiding this comment

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

(Why was this switched in the first place?)

Copy link
Member

Choose a reason for hiding this comment

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

My guess is that, since the table/memory index was always 0, it made sense to put it at the end (e.g. similar to call_indirect)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I agree with @rossberg re the operand order in the text format.

One thing that annoys me about the text format though is that in the elem segment I must write (table $t) while in call_indirect, table.copy, table.init, etc, I can only write $t. If the text format would admit disambiguating syntax it could also admit a more flexible operand order: (table.init (elem 1) (table $t) ...). Something for another day perhaps.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that would be piggybacking on the wrong thing, since that syntax would only happen to work where the immediates are of different sort, e.g. not table.copy. Moreover, for various instructions, src and dst are denoted by a combination of immediates and stack operands, and their order is supposed to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that's true for the order of the table operands. It's still annoying that the syntax is non-uniform, but it's just a hobbyhorse - not a problem to solve here.

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Oops, sorry, missed this.

Thanks!

@rossberg rossberg merged commit 1568b35 into WebAssembly:master Mar 24, 2020
rossberg added a commit that referenced this pull request May 14, 2020
* Remove passive segment func ref shorthand

* Drop passive keyword
rossberg pushed a commit that referenced this pull request May 14, 2020
Change the test generators to use `ref.func` and remove `passive`.

At some point we'll want to remove the generators, but for let's try to
maintain them.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[test] Bulk instructions with multiple tables
3 participants