Generate binary tests for relaxed atomics #8245
Conversation
stevenfontanella
force-pushed
the
binary-tests
branch
2 times, most recently
from
2a87bc9 to
95fc507
Compare
stevenfontanella
force-pushed
the
binary-tests
branch
from
95fc507 to
01a2a96
Compare
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Nice! Just a couple comments.
|
|
||
| def statement(template, mem_idx: str | None, ordering: Ordering | None): | ||
| def all_combinations(): | ||
| return itertools.product(templates, [None, 0, 1], [None, Ordering.acqrel, Ordering.seqcst]) |
There was a problem hiding this comment.
It would be good to add a comment about what is being combined here. Also, we could use ValueType.i32 and ValueType.i64 instead of 0 and 1 here to make things clearer.
There was a problem hiding this comment.
Added a comment and extra arguments for the memory type in addition to the memory index. I think both are needed, since semantically this is a memory index, and we also use None which maps to i32 (because memory 0 is an i32 memory), which wouldn't be clear on its own.
There was a problem hiding this comment.
Oh, I see, the 0 and 1 are actual memory indices. I misunderstood and thought they were just representing whether we were indexing a 32-bit or 64-bit memory. Just including one bit of information, whether the memory index or memory type, along with the None in all_combinations sounds fine to me.
| Template(op="i32.atomic.load", value_type=ValueType.i32, args=1, should_drop=True, bin=b"\xfe\x10"), | ||
| Template(op="i64.atomic.load", value_type=ValueType.i64, args=1, should_drop=True, bin=b"\xfe\x11"), | ||
| Template(op="i32.atomic.store", value_type=ValueType.i32, args=2, should_drop=False, bin=b"\xfe\x17"), | ||
| Template(op="i64.atomic.store", value_type=ValueType.i64, args=2, should_drop=False, bin=b"\xfe\x18"), |
There was a problem hiding this comment.
In the long run (i.e. once we generate executable tests) I expect we will want to associate each operator with a function type like ([ValueType.i32, ValueType.i32, ValueType.indexType], [ValueType.i32]) so we can export a function with the correct signature for each operation. We could replace value_type and should_drop with those signatures now to get ahead of that.
There was a problem hiding this comment.
Would it be ok to defer this until we're writing the execution tests? The argument for the index type depends on the memory type which we don't have here (we loop over it later along with the template type). It's likely that I'd write a different script for execution tests but possibly share some of the code from this script.
rmahdav
left a comment
rmahdav
left a comment
There was a problem hiding this comment.
LGTM, thanks!
(I was going to ask why you did not remove the existing tests but it looks like you are doing it in the follow-up PR.)
Generate the equivalent of the existing text tests, with additional comments describing each piece of the binary.
The remaining TODOs are to add support for RMW operations (which encode memory ordering slightly differently) and add the remaining atomic operations to the test.
Part of #8165.