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.

Sign up for GitHub

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

Jump to bottom

[interpreter] Unify assert_result* assertions #1104

Merged
merged 1 commit into from
Dec 9, 2019
Merged

[interpreter] Unify assert_result* assertions #1104

merged 1 commit into from
Dec 9, 2019

Conversation

rossberg
Copy link
Member

Implement the base version of the assertion refactoring discussed here, unifying assert_return_*_nan with the regular assert_return by generalising the result literals of the latter to a simple form of pattern for result values:

result ::= (t.const pattern*)
pattern ::= literal | nan:canonical | nan:arithmetic

Here, pat extends the syntax of literals in const instructions with additional patterns that extend the existing nan-with-payload syntax. This could be extended to other non-deterministic value classes.

Motivation:

  1. This makes more sense for multiple return values, which can now use different patterns for individual components.
  2. Extends better to SIMD, which would extend the result pattern to allow different patterns for individual lanes.
  3. Extends better to other proposals, such as reference types, which could further extend the syntax of result with additional patterns, such as (ref.any).

@rossberg rossberg mentioned this pull request Nov 19, 2019
Closed
@rossberg rossberg requested a review from tlively November 19, 2019 19:46
@tlively
Copy link
Member

tlively commented Nov 20, 2019

I can't say my OCaml is up to snuff, but this solution LGTM.

@rossberg
Copy link
Member Author

@abrown, for some reason I cannot add you as a reviewer, but please take a look.

abrown
abrown reviewed Nov 20, 2019
View reviewed changes
interpreter/text/parser.mly

result_list :
| /* empty */ { [] }
| result result_list { $1 :: $2 }
Copy link

Choose a reason for hiding this comment

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

Should the result list be inside parens? Right now, when there are multiple return values specified, it seems like we would write assert_return (invoke ...) result1 result2 result3) but should it instead be assert_return (invoke ...) (result1 result2 result3))? (Asking, not claiming it should 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.

That's already the case in the current format. It is symmetric to the argument list, which is not being parenthesised either:

(assert_return (invoke "f" arg1 arg2 arg3) res1 res2 res3)

In fact, parens there would require inserting another keyword as head.

abrown
abrown approved these changes Nov 20, 2019
View reviewed changes
Copy link

@abrown abrown left a comment

Choose a reason for hiding this comment

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

This is awesome (and much faster than I could have done!). I had one question above and I don't understand what the build failure is about, but it looks good to me.

@rossberg
Copy link
Member Author

@abrown, thanks. Could you approve the PR?

There is no build failure, it's just the automatic IPR checker. For some reason, it doesn't recognise my GH account anymore. I should probably try to figure out why some day...

@abrown
Copy link

abrown commented Dec 2, 2019

I approved but my approval must not count for much ☹️. @tlively are you able to approve and merge?

@tlively
Copy link
Member

tlively commented Dec 2, 2019

LGTM, thanks for taking a closer look @abrown! I'll let @rossberg merge when he's ready.

@Honry Honry mentioned this pull request Dec 5, 2019
Closed
@abrown
Copy link

abrown commented Dec 6, 2019

@rossberg, ping

This was referenced Dec 6, 2019
Merged
Merged
@rossberg
Copy link
Member Author

rossberg commented Dec 9, 2019

Sorry for the delay, I was OOO.

@rossberg rossberg merged commit e3a3c12 into master Dec 9, 2019
@rossberg rossberg deleted the resultpat branch December 9, 2019 08:05
@AndrewScheidecker AndrewScheidecker mentioned this pull request Dec 20, 2019
Merged
satabin added a commit to satabin/swam that referenced this pull request Dec 25, 2019
@satabin
Unify assert_result assertions
2b34235 
See PR WebAssembly/spec#1104
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Dec 26, 2019
@sbc100
Update testsuite
6c71f83 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.
@sbc100 sbc100 mentioned this pull request Dec 26, 2019
Merged
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Dec 26, 2019
@sbc100
Update testsuite
c9d15d2 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Dec 26, 2019
@sbc100
Update testsuite
b4c17b9 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
@abrown abrown mentioned this pull request Jan 8, 2020
Closed
4 tasks
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Jan 9, 2020
@sbc100
Update testsuite
812c5ee 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Jan 9, 2020
@sbc100
Update testsuite
91269d8 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Jan 9, 2020
@sbc100
Update testsuite
f48eabd 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Jan 9, 2020
@sbc100
Update testsuite
6335128 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.

3. nullref is now permitted in the text and binary format.
sbc100 added a commit to WebAssembly/wabt that referenced this pull request Jan 9, 2020
@sbc100
Update testsuite (#1275)
415c3bb 
The two primary changes involved are:

1. Removal of `assert_return_canonical_nan`/`arithetic nan` in favor of
   special `nan:canonical`/`nan:arithmetic` constants that can only be
   used in test expectations.
   See: WebAssembly/spec#1104

2. New trapping behaviour for bulk memory operations.  Range checks are
   now performed up front for opterations such as memory.fill and
   memory.copy.
   See: WebAssembly/bulk-memory-operations#111
   And: WebAssembly/bulk-memory-operations#123
   The old behaviour is still kept around to support table.fill which
   is defined in reference-types proposal and has yet to be updated.
abrown added a commit to abrown/wasmtime that referenced this pull request Jan 10, 2020
@abrown
Update wast to 6.0.0
8026303 
Due to API changes in wast, this removes all AssertReturn* directives and expands the functionality the single AssertReturn directive with the `nan:canonical` and `nan:arithmetic` patterns. See bytecodealliance/wat#42 and WebAssembly/spec#1104 for context.
@abrown abrown mentioned this pull request Jan 10, 2020
Closed
@abrown abrown mentioned this pull request Mar 27, 2020
Closed
@binji binji mentioned this pull request Apr 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abrown abrown abrown approved these changes

@tlively tlively tlively approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rossberg @tlively @abrown