Skip to content

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Feb 15, 2025

Change SimulatorAPI methods to use the ChiselSettings class instead of
directly communicating layerControl. This will enable us to lump together
more options that all are associated with Chisel/FIRRTL/the Verilog ABI.

Extend ChiselSettings with three macros which were currently being set by
ChiselRunners:

  • ASSERT_VERBOSE_COND
  • PRINTF_COND
  • STOP_COND

Make this work in such a way that the user can specify a function that
will return the signal that they want to use to set this macro in one of
several predefined ways.

This is working towards replacing this part of ChiselSpec: https://github.com/chipsalliance/chisel/blob/main/src/test/scala-2/chiselTests/ChiselSpec.scala#L60

Release Notes

  • Add FIRRTL/Verilog ABI macro control to Chiselsim.

@seldridge seldridge added the Feature New feature, will be included in release notes label Feb 15, 2025
layerControl: LayerControl.Type = LayerControl.EnableAll,
chiselSettings: ChiselSettings[T] = ChiselSettings.default[T],
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: because the default factory is necessarily given a type parameter with a lower bound (A <: RawModule]), this necessitates giving it a type parameter on line 77 here. If I don't do this, this will infer to Nothing and everything blows up.

There may be a way to untangle this.

@seldridge seldridge marked this pull request as ready for review February 15, 2025 04:10
@seldridge seldridge requested a review from jackkoenig February 15, 2025 04:11
@seldridge seldridge force-pushed the dev/seldridge/chiselsim-chisel-settings branch from 50f6ddf to 58a1586 Compare February 19, 2025 05:55
@seldridge seldridge changed the base branch from main to dev/seldridge/switch-to-ChiselSettings February 19, 2025 19:37
@seldridge seldridge force-pushed the dev/seldridge/chiselsim-chisel-settings branch from 58a1586 to bd1218d Compare February 19, 2025 19:38
Base automatically changed from dev/seldridge/switch-to-ChiselSettings to main February 19, 2025 19:55
@seldridge seldridge force-pushed the dev/seldridge/chiselsim-chisel-settings branch from bd1218d to 16ec7fe Compare February 20, 2025 05:08
Extend ChiselSettings with three macros which were currently being set by
ChiselRunners:

  - ASSERT_VERBOSE_COND
  - PRINTF_COND
  - STOP_COND

Make this work in such a way that the user can specify a function that
will return the _signal_ that they want to use to set this macro in one of
several predefined ways.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge force-pushed the dev/seldridge/chiselsim-chisel-settings branch from 16ec7fe to ac56d63 Compare February 21, 2025 22:07
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

LGTM! A few nits but I like it

@seldridge seldridge enabled auto-merge (squash) February 22, 2025 03:17
@seldridge seldridge merged commit cbaa302 into main Feb 22, 2025
15 checks passed
@seldridge seldridge deleted the dev/seldridge/chiselsim-chisel-settings branch February 22, 2025 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants