[test] Add spec tests for integer min/max ops #163
Conversation
Tests are pulled from WAVM/WAVM#242
|
Let's wait to merge until we get a second set of eyes on the python changes. |
| if '0x' in p1: | ||
| base1 = 16 | ||
| else: | ||
| base1 = 10 |
There was a problem hiding this comment.
It looks like the parameters are allowed to be base-10, but the doc comment specifies that they must be hex. One of these should be updated.
There was a problem hiding this comment.
Oh, I will fix it soon.
| elif op in ['min_u', 'max_u']: | ||
| lane = LaneValue(lane_width) | ||
| i1 = v1 & lane.mask | ||
| i2 = v2 & lane.mask |
There was a problem hiding this comment.
Why use IntegerSimpleOp.get_valid_value above but & here? It might be cleaner to use the same kind of interface for signed and unsigned values, even if underneath the covers the unsigned implementation is simpler than the signed implementation.
There was a problem hiding this comment.
Good proposal, I will improve it.
| def binary_op(op: str, p1: str, p2: str, lane_width: int) -> str: | ||
| """Binary operation on p1 and p2 with the operation specified by op | ||
|
|
||
| :param op: min_s, min_u, max_s, max_u |
There was a problem hiding this comment.
This seems generally useful. Are you planning to extend this to more operations?
There was a problem hiding this comment.
Yeah, it's part of my ongoing plan.
|
|
||
| @property | ||
| def quarter(self): | ||
| return pow(2, self.lane_width - 2) No newline at end of file |
There was a problem hiding this comment.
I'm splitting this class from https://github.com/WebAssembly/simd/blob/master/test/core/simd/meta/simd_arithmetic.py#L20, which is used for set various test data per properties defined above.
e.g. for an i8 value, its bit width is 255, its quarter is 64, for an i16 value, its quarter is 16384, thus we can have different test data for different lane width value. I just want to specific a test data between the boundary value.
test/core/simd/simd_i16x8.wast
Outdated
| ) | ||
| ) | ||
| "type mismatch" | ||
| )(assert_invalid |
There was a problem hiding this comment.
This is a little funky without an extra newline in there.
There was a problem hiding this comment.
Ummm... I will figure it out.
| @@ -0,0 +1,387 @@ | |||
| ;; Tests for i16x8 [min_s, min_u, max_s, max_u] operations. | |||
There was a problem hiding this comment.
Should the file name include "min" and "max" or is it going to eventually include many more operations?
There was a problem hiding this comment.
I will include the "avgr" then, don't have an ideal file name, any proposal?
There was a problem hiding this comment.
Not any good ideas. The best I can think of is simd_i16x8_val_arith.wast. Adding "val" doesn't really convey much more information, but it does suggest that the contents are more specific than the name simd_i16x8.wast suggests. My other idea is simd_i16x8_arith2.wast, which has the same benefits and drawbacks.
There was a problem hiding this comment.
I'd prefer simd_i16x8_arith2.wast. :)
| "type mismatch" | ||
| ) | ||
|
|
||
| ;; Combination |
There was a problem hiding this comment.
How are we deciding what combinations of instructions are useful to test?
There was a problem hiding this comment.
Good question, actually I don't have a specific definition, I just choose some combinations randomly.
And we don't want to leverage full set of pairwise comparison, since it will cause exponential test cases with low efficiency to capture bugs.
There was a problem hiding this comment.
Sounds good. I would err on the side of testing fewer combinations because I agree that we want to avoid having more tests than necessary and I don't think the combinations will find as many bugs.
| def binary_op(op: str, p1: str, p2: str, lane_width: int) -> str: | ||
| """Binary operation on p1 and p2 with the operation specified by op | ||
|
|
||
| :param op: min_s, min_u, max_s, max_u |
There was a problem hiding this comment.
Yeah, it's part of my ongoing plan.
| if '0x' in p1: | ||
| base1 = 16 | ||
| else: | ||
| base1 = 10 |
There was a problem hiding this comment.
Oh, I will fix it soon.
| elif op in ['min_u', 'max_u']: | ||
| lane = LaneValue(lane_width) | ||
| i1 = v1 & lane.mask | ||
| i2 = v2 & lane.mask |
There was a problem hiding this comment.
Good proposal, I will improve it.
|
|
||
| @property | ||
| def quarter(self): | ||
| return pow(2, self.lane_width - 2) No newline at end of file |
There was a problem hiding this comment.
I'm splitting this class from https://github.com/WebAssembly/simd/blob/master/test/core/simd/meta/simd_arithmetic.py#L20, which is used for set various test data per properties defined above.
e.g. for an i8 value, its bit width is 255, its quarter is 64, for an i16 value, its quarter is 16384, thus we can have different test data for different lane width value. I just want to specific a test data between the boundary value.
| @@ -0,0 +1,387 @@ | |||
| ;; Tests for i16x8 [min_s, min_u, max_s, max_u] operations. | |||
There was a problem hiding this comment.
I will include the "avgr" then, don't have an ideal file name, any proposal?
test/core/simd/simd_i16x8.wast
Outdated
| ) | ||
| ) | ||
| "type mismatch" | ||
| )(assert_invalid |
There was a problem hiding this comment.
Ummm... I will figure it out.
| "type mismatch" | ||
| ) | ||
|
|
||
| ;; Combination |
There was a problem hiding this comment.
Good question, actually I don't have a specific definition, I just choose some combinations randomly.
And we don't want to leverage full set of pairwise comparison, since it will cause exponential test cases with low efficiency to capture bugs.
|
Thanks, @Honry! I'll merge this now, but feel free to change the filename in a followup PR if you want. |
3 participants
Tests are pulled from WAVM/WAVM#242