Skip to content

[interpreter] Unify assert_result* assertions#1104

Merged
rossberg merged 1 commit intomasterfrom
resultpat
Dec 9, 2019
Merged

[interpreter] Unify assert_result* assertions#1104
rossberg merged 1 commit intomasterfrom
resultpat

Conversation

@rossberg
Copy link
Member

@rossberg rossberg commented Nov 19, 2019

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

rossberg commented Nov 20, 2019

@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

@abrown abrown Nov 20, 2019

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

@rossberg rossberg Nov 21, 2019

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

rossberg commented Nov 21, 2019

@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
raoxiaojia pushed a commit to WasmCert/spec that referenced this pull request Apr 29, 2025
@rossberg
[interpreter] Unify assert_result* assertions (WebAssembly#1104)
e014bdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tlively tlively tlively approved these changes

+1 more reviewer

@abrown abrown abrown approved these changes

Reviewers whose approvals may not affect merge requirements

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