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

Scala 3 Cross-Compilation Support #14

Merged
merged 10 commits into from
May 10, 2023
Merged

Scala 3 Cross-Compilation Support #14

merged 10 commits into from
May 10, 2023

Conversation

vighneshiyer
Copy link
Contributor

Cross-compile firrtl for Scala 2.13 and 3. I'm slowing adding the necessary changes to this PR to see when a test breaks.

Copy link
Collaborator

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

Looks good to me as long as it passes CI

Copy link
Collaborator

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

The CI needs to also run the Scala 3 version.

@vighneshiyer
Copy link
Contributor Author

https://github.com/ucb-bar/firrtl2/actions/runs/4931578837/jobs/8813751741?pr=14#step:9:1198

It seems that either there is a regression in yosys, or the constant propagation pass has a real bug that wasn't caught with an old version of yosys. I will run equivalence checking using Jasper to figure out which one.

@ekiwi
Copy link
Collaborator

ekiwi commented May 10, 2023

It seems that either there is a regression in yosys, or the constant propagation pass has a real bug that wasn't caught with an old version of yosys. I will run equivalence checking using Jasper to figure out which one.

That is a good idea. I think yosys equivalence checks can be a bit brittle sometimes.

@vighneshiyer
Copy link
Contributor Author

firrtl_ops.zip

Run jasper with jg -allow_unsupported_OS -no_gui -sec -tcl equiv_check.tcl.

INFO: Verification result is: NOT 'proven'. The designs are NOT equivalent.

There is a bug in the firrtl pass, but I'm not sure where to start debugging it.

@ekiwi
Copy link
Collaborator

ekiwi commented May 10, 2023

There is a bug in the firrtl pass, but I'm not sure where to start debugging it.

Can you compare to the firrtl output before you applied your changes? Does that pass with Jasper?

@vighneshiyer
Copy link
Contributor Author

The firrtl output (Ops_custom.v and Ops_ref.v) before and after I applied my changes is identical. CI also confirms that the equivalence check passed before and after my changes on an old version of yosys, but a new version of yosys catches the non-equivalence.

@ekiwi
Copy link
Collaborator

ekiwi commented May 10, 2023

The firrtl output (Ops_custom.v and Ops_ref.v) before and after I applied my changes is identical. CI also confirms that the equivalence check passed before and after my changes on an old version of yosys, but a new version of yosys catches the non-equivalence.

If that is the case then I would suggest that you don't update the yosys version in this PR since the failure is unrelated. Once we land Scala 3 support you can make another PR that updates the yosys version and we can discuss debugging the bug on that PR.

@vighneshiyer
Copy link
Contributor Author

Sounds good, I reverted the OSS CAD suite version bump, and added Scala 3 to the CI matrix. I expect 2 JSON parser related tests to fail in CI for Scala 3, and we can discuss how to fix those soon.

@vighneshiyer
Copy link
Contributor Author

Looks like CI hits the 2 JSON-related unittest failures. This seems to be an issue in how json4s handles extraction of generic types (e.g. List). The issue is documented here: json4s/json4s#1035

What do you suggest? Should I try using a different JSON library, or is the surface that touches too large? If there are no/few annotations that use these features, perhaps these specific unittests can be ignored for Scala 3?

@ekiwi ekiwi marked this pull request as ready for review May 10, 2023 14:10
Copy link
Collaborator

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

I removed the JSON tests. I think the feature tested there is rare. Mostly used by Firesim.

@ekiwi ekiwi merged commit 62db135 into main May 10, 2023
9 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.

None yet

2 participants