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

Refactored tick() in simulator.dart #475

Merged
merged 5 commits into from
Apr 3, 2024
Merged

Conversation

AdamRose66
Copy link
Contributor

@AdamRose66 AdamRose66 commented Mar 15, 2024

Description & Motivation

Refactored tick() method in simulator.dart.

The high level motivation is to move towards a common scheduler shared by the Rohd and Rohm simulators. This is preparatory refactoring with that in mind.

Concretely, I have added a method for each phase.
- all phase methods update _phase and add an event to their controller to let listeners know the new phase is now in progress
- some phases are synchronous and not awaited
- other phases are asychronous and are awaited
- the asynchronous phases do something important before or after transitioning to the phase in question

The tick method now looks like this:

  static Future<void> tick() async {
    if (_injectedActions.isNotEmpty) {
      _pendingTimestamps.putIfAbsent( _currentTimestamp , () => ListQueue() );
    }

    // the main event loop
    if (_updateTimeStamp()) {
      _preTick();
      await _mainTick();
      _clkStable();
      await _outOfTick();
    }
  }

The justification is as follows:

       // case 1 : ( the usual Rohd case )
       //   The previous delta cycle did NOT do 'registerAction( _currentTimeStamp );'.
       //   In that case, _pendingTimestamps[_currentTimestamp] is currently
       //   null, so putIfAbsent will add a new empty list, which will trigger
       //   a new delta cycle.
       // case 2 :
       //   The previous delta cycle DID do 'registerAction( _currentTimestamp );'.
       //   In that case, there is *already* another tick scheduled for
       //   _currentTimestamp, and the injected actions will get called in
       //   the normal way.
       ...
      // Either way, the end result is that a whole new tick gets scheduled for
      // _currentTimestamp and any outstanding injected actions get executed.

Testing

All Rohme and Rohd tests pass using this code.

Backwards-compatibility

The user facing API has not changed and tool integrations should be unaffected.

Documentation

Documentation is inline ( including for the new private methods ).

    - added phase methods, some of which are awaited.
    - all phase methods update _phase and add an event to their controller
    - some phases add functionality before or after the phase update
    - made the actual requirements for injected actions more explicit in the code
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Overall, this looks like a reasonable change and makes the code look cleaner, thank you for taking the time on this!

lib/src/simulator.dart Outdated Show resolved Hide resolved
lib/src/simulator.dart Show resolved Hide resolved
injected action scheduling.

See extended comment in Simulator.tick() for detailed explanation.
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

I think this looks good to me, thank you again for the contribution!

I ran it against the rohd-cosim test suite and all passes. I filed a separate issue (#479) tracking adding more testing here.

Just need to fix up the failing CI error(s)

@AdamRose66
Copy link
Contributor Author

Checks now pass but build dev tools fails, not sure why or what to do about that.

@AdamRose66
Copy link
Contributor Author

AdamRose66 commented Apr 3, 2024 via email

@mkorbel1 mkorbel1 merged commit 7bf9f0c into intel:main Apr 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants