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.

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

RFC 36 (async/await testing) #742

Merged
merged 52 commits into from
Nov 19, 2024
Merged

RFC 36 (async/await testing) #742

merged 52 commits into from
Nov 19, 2024

Conversation

tilk
Copy link
Member

@tilk tilk commented Oct 29, 2024

This PR refactors the tests for Amaranth's RFC 36. It is pretty massive, so thorough reviews are not expected. Let's merge this as quickly as possible and then deal with the possible aftermath later.

Important changes:

  • In case when synchronization between different processes is actually needed, uses of Settle were replaced with sim.delay().
  • Other instances of Settle were removed.
  • Signal sampling (sim.tick().sample()) is used instead of signal value getting where it was reasonably simple to do so. In other cases, sim.get() is used (but we should try to avoid it).
  • Method mocks work differently:
    • They consist of multiple processes: a sequential process, which works like before, plus combinational processes for updating the circuit after combinational changes. This helps avoid races in case of combinational dependencies between mocked methods.
    • Because the method mock can now be called multiple times per cycle, side effects must be guarded by MethodMock.effect. The effects are run once per cycle.
  • For testbench processes which manipulate multiple methods in synchrony, CallTrigger was added, which allows to call multiple methods in a single clock cycle, together with sampling signals and the state of other methods.
  • Number of ticks from simulation start can now be accessed by using a counter signal under TicksKey().
  • Update testing abstractions
  • Refactor tests
  • Fix documentation
  • Cleanup

@tilk tilk added tests Tests and testbenches (not infrastructure) refactor Doesn't change functionality, but makes stuff nicer labels Oct 29, 2024
@tilk tilk force-pushed the tilk/async-tests branch from 94ba441 to 76727d0 Compare October 30, 2024 12:00
@tilk tilk force-pushed the tilk/async-tests branch from d6dc0e1 to d260186 Compare November 6, 2024 12:46
@tilk tilk marked this pull request as ready for review November 13, 2024 14:39
@tilk tilk mentioned this pull request Nov 14, 2024
Copy link
Contributor

@lekcyjna123 lekcyjna123 left a comment

Choose a reason for hiding this comment

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

Only some bikeshedding.

transactron/testing/method_mock.py Outdated Show resolved Hide resolved
transactron/testing/method_mock.py Outdated Show resolved Hide resolved
transactron/testing/method_mock.py Outdated Show resolved Hide resolved
transactron/testing/testbenchio.py Show resolved Hide resolved
transactron/testing/testbenchio.py Outdated Show resolved Hide resolved
async for _, _, ticks_val, combined_trigger_val, *record_vals in (
sim.tick("sync_neg")
.sample(ticks, combined_trigger)
.sample(*itertools.chain(*([record.trigger] + record.fields for record in records)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This equals to .sample(records[0].trigger, records[0].fields[0], ..., records[0].fields[-1], records[1].trigger, records[1].fields[0], ..., records[1].fields[-1], ...), do I understand right? It fits with logic inside handle_logs.

Looks a bit contrived, maybe some conversion could be extracted to a helper function, so that the records are grouped somehow maybe. Can handle_logs sample the records on its own?

Copy link
Member Author

Choose a reason for hiding this comment

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

Had the exact same thought. To do it better, you would probably need to represent the records using amaranth.lib.data or create custom ShapeCastables, ValueCastables, etc. Each record is different, so each would need a different shape. I decided to use this solution because it seemed to need less work than the alternative. I can leave a comment if you think it's warranted.

test/transactron/testing/test_validate_arguments.py Outdated Show resolved Hide resolved
test/func_blocks/fu/common/test_rs.py Show resolved Hide resolved
test/func_blocks/fu/fpu/test_fpu_error.py Show resolved Hide resolved
test/func_blocks/fu/fpu/test_fpu_error.py Show resolved Hide resolved
@tilk tilk merged commit 75c2706 into master Nov 19, 2024
13 checks passed
@tilk tilk deleted the tilk/async-tests branch November 19, 2024 09:31
github-actions bot pushed a commit that referenced this pull request Nov 19, 2024
tilk added a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
@whitequark
Copy link

@tilk How was your experience with async/await processes/testbenches? Any pain points I should know about?

@tilk
Copy link
Member Author

tilk commented Dec 19, 2024

@whitequark Pretty good actually. The sampling mechanism helps with races quite well. However, porting some tests required more work, because sampling requires you to take samples on a tick; you cannot decide later that you wanted to have a sample of some signal. A variant of get which returns the value of the signal that was there on the last tick would increase flexibility, but I imagine this might be hard to implement without losing some of the simulation performance.

It would be nice to have some synchronization mechanism which could allow removing races on Python state shared between processes. Currently I solve this by (ab)using sim.delay() - this is a good example.

@whitequark
Copy link

Very happy that you like it! The plan is to treat the new simulation mechanism as stable and not ever have as much churn as this migration caused, so I'm glad that it works well. A lot of careful work went into it.

A variant of get which returns the value of the signal that was there on the last tick would increase flexibility, but I imagine this might be hard to implement without losing some of the simulation performance.

Yes. Sampling a value essentially sets up a "tripwire" which then calls .get and remembers the result. Copying the entire simulator state is of course technically feasible but seems very wasteful. We considered it and decided not to add it at the time. Of course an RFC could change that.

It would be nice to have some synchronization mechanism which could allow removing races on Python state shared between processes.

I agree! At the time, my "solution" to it is to use Amaranth signals to share state, but this of course does not cover everything. It comes to mind that you could probably make a "mutex" (or other sync primitives) out of an Amaranth signal and random Python data. Would this solve anything?

Ideally I'd like to expose an easier to use concept, but I don't have any thoughts on the design.

@whitequark
Copy link

(When building a mutex there comes the issue of two processes locking it at exactly the same time. I'm thinking that each lock() call site could allocate a bit while spinning and the lowest set bit wins. It's kind of a weird way to construct a primitive, but you probably have parallels to this in the CPU itself, don't you?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Doesn't change functionality, but makes stuff nicer tests Tests and testbenches (not infrastructure)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants