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

Get ChiselSim working with CIRCT 1.66+ #3890

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Mar 1, 2024

Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist that includes all modules, including blackboxes, in the design. Change this to try to also copy files from the black box filelist. This makes ChiselSim work with CIRCT 1.66 and earlier (which put everything in 'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in 'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock solid ABI that is implemented by CIRCT where all modules can be discovered under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

This fixes the nightly failure seen here: https://github.com/chipsalliance/chisel/actions/runs/8110395678

Release Notes

Change ChiselSim to also move blackboxes as indicated by the default black box filelist. This fixes issues with ChiselSim and CIRCT 1.67.0 introduced in (llvm/circt#6729). This is a backwards compatible change that has no effect on earlier versions of CIRCT.

Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist
that includes all modules, _including blackboxes_, in the design.  Change
this to try to also copy files from the black box filelist.  This makes
ChiselSim work with CIRCT 1.66 and earlier (which put everything in
'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in
'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock
solid ABI that is implemented by CIRCT where all modules can be discovered
under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge added the Bugfix Fixes a bug, will be included in release notes label Mar 1, 2024
@seldridge
Copy link
Member Author

@jackkoenig I'm going to merge this and ask for post-merge review (as this is holding up some other things).

@seldridge seldridge merged commit 9448d32 into main Mar 1, 2024
17 checks passed
@seldridge seldridge deleted the dev/seldridge/fix-CIRCT-1.67.0 branch March 1, 2024 16:20
@jackkoenig
Copy link
Contributor

@seldridge does this work with older version of firtool? Can/should we backport this to 5.x?

@jackkoenig
Copy link
Contributor

RTFM, yes:

This is a backwards compatible change that has no effect on earlier versions of CIRCT.

Comment on lines +130 to +135
// Move a file in a filelist which may not exist.
def maybeMoveFiles(filename: String) = try {
moveFiles(filename)
} catch {
case _ @(_: java.nio.file.NoSuchFileException | _: java.io.FileNotFoundException) =>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to write this as "check if exists, then move if so" but whatever, very Pythonic if you 😉

@jackkoenig jackkoenig added this to the 5.x milestone Mar 1, 2024
@mergify mergify bot added the Backported This PR has been backported label Mar 1, 2024
mergify bot pushed a commit that referenced this pull request Mar 1, 2024
Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist
that includes all modules, _including blackboxes_, in the design.  Change
this to try to also copy files from the black box filelist.  This makes
ChiselSim work with CIRCT 1.66 and earlier (which put everything in
'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in
'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock
solid ABI that is implemented by CIRCT where all modules can be discovered
under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 9448d32)
mergify bot pushed a commit that referenced this pull request Mar 1, 2024
Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist
that includes all modules, _including blackboxes_, in the design.  Change
this to try to also copy files from the black box filelist.  This makes
ChiselSim work with CIRCT 1.66 and earlier (which put everything in
'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in
'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock
solid ABI that is implemented by CIRCT where all modules can be discovered
under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 9448d32)
@seldridge
Copy link
Member Author

I tested it with 1.66.0 and assumed that was enough to setup the inductive proof... (Empirically this works on 1.66 as this passed CI and works on the 1.67 candidate as it passed nightly after I manually ran it.)

This may, however, be working because of the aggressive exception hiding in maybeMoveFiles now that I think about it. maybeMoveFiles is catching java.nio.file.NoSuchFileException and java.io.FileNotFoundException. What I think is going on is that in 1.67, this will only ever hit one of these if when it fails to find the black box resource filelist. However, in 1.66, the blackbox files may be duplicated in both the black box resource filelist and the main filelist. This means that this is working because it is catching both the case of the black box filelist not existing and the case of the files already having been moved.

tl;dr: empirically this should be safe to backport. The exception catching logic may be a little jank.

@jackkoenig
Copy link
Contributor

If we need to tweak it on 5.x or 6.x that's fine, thanks for getting the fix in! We can tweak the exception logic later, nbd.

chiselbot pushed a commit that referenced this pull request Mar 1, 2024
Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist
that includes all modules, _including blackboxes_, in the design.  Change
this to try to also copy files from the black box filelist.  This makes
ChiselSim work with CIRCT 1.66 and earlier (which put everything in
'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in
'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock
solid ABI that is implemented by CIRCT where all modules can be discovered
under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 9448d32)

Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
chiselbot pushed a commit that referenced this pull request Mar 1, 2024
Fix an incorrect assumption in ChiselSim that a 'filelist.f' will exist
that includes all modules, _including blackboxes_, in the design.  Change
this to try to also copy files from the black box filelist.  This makes
ChiselSim work with CIRCT 1.66 and earlier (which put everything in
'filelist.f') and CIRCT 1.66+ (which does not include blackboxes in
'filelist.f').

Note that this is intended as a temporary solution until FIRRTL has a rock
solid ABI that is implemented by CIRCT where all modules can be discovered
under an arbitrary public module.

See CIRCT changes here: llvm/circt#6729

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 9448d32)

Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants