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

tests: add murax test #17

Merged
merged 8 commits into from
Jun 11, 2021
Merged

Conversation

acomodi
Copy link
Contributor

@acomodi acomodi commented May 10, 2021

Signed-off-by: Alessandro Comodi [email protected]

This adds the Murax test design.

TODO:

  • Fix invalid site thrus
  • Add a proper testbench if possible.
  • Fix increased runtime during routing.

cc @gatecat: for the routing run-time increase (which is pretty substantial) I have isolated the problem being in the changes added with this PR: YosysHQ/nextpnr#697. It is still unclear to me what might have caused a spike in the murax routing times, but reverting those changes resulted in router time to go back to "normal"

@acomodi acomodi marked this pull request as draft May 10, 2021 17:26
@gatecat
Copy link
Contributor

gatecat commented May 10, 2021

I'll look at the Murax issue, that PR significantly improved performance for Nexus and I suspect the increase for interchange is a latent issue of some kind (probably very poor lookahead).

@acomodi
Copy link
Contributor Author

acomodi commented May 11, 2021

With @gatecat's fix to the bounding box (YosysHQ/nextpnr#702) and the workarounds listed below we got to a Murax design working on HW.

Remaining workarounds to solve:

  • Add proper way of allowing legal site routes during general router. Workaround: interchange: arch: do not allow site pips within sites YosysHQ/nextpnr#700
  • Do not allow A6 -> O6 route-thrus when LUT5 is used. Workaround: remove A6 -> O6 pseudo-pips.
  • Add timing data. It appears that the even with dedicated clock routing, we hit timing clousure issues. The basys3 has a 100MHz clock, which resulted in timing not being met (hitting a -20ms setup violations). Workaround: divide the clock to 25MHz everything seems to be working fine.

read_verilog $src
}

synth_xilinx -flatten -nolutram -nowidelut -nosrl -nocarry -nodsp
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these flags should be located somewhere other than the individual tests? So that as we support more features, we don't have to update all the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definetely, I'd actually say that the whole run.tcl file might end up in the common directory

@gatecat
Copy link
Contributor

gatecat commented May 11, 2021

As you mentioned in the other issue, supporting carry chains will also make a big impact on Fmax, possibly almost as significant as adding proper timing data.

acomodi added 3 commits May 14, 2021 15:38
Signed-off-by: Alessandro Comodi <[email protected]>
Also add synth and pnr logs

Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
Copy link
Contributor Author

acomodi commented May 14, 2021

This PR requires a new conda package with the fixes for the illegal tile PIPs.

The murax design was locally tested and pushed to work at 50MHz.

@@ -1,7 +1,14 @@
// Copyright (C) 2021 The Symbiflow Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

ianal, but I'm not convinced this copyright header is correct - any copyright applying to this would be a derivative of VexRiscv/Murax's copyright. But probably the license checker should just have a way of skipping generated files, so we don't have to remember to patch on a copyright header if updating it.

Copy link
Contributor Author

@acomodi acomodi May 17, 2021

Choose a reason for hiding this comment

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

Yeah, this was not intended to get checked in, but to get CI past the error. Actually, given that kokoro does not perform the license checking, I can just remove this commit, as I wanted to double-check that everything is all right in CI as well. But yeah, the license checker should avoid generated files to get checked.

@acomodi
Copy link
Contributor Author

acomodi commented May 17, 2021

the Murax design was successful, with no errors generated during bitstream generation. The bitstream runs on HW@50MHz.

There is a known issue though on the FASM generation which affects the ram test (and most probably also the Murax test), for which LUT-thrus corresponding to pseudo tile PIPs are not correctly recognized.

@acomodi acomodi changed the title WIP: tests: add murax test tests: add murax test May 21, 2021
@acomodi acomodi marked this pull request as ready for review May 21, 2021 09:39
@acomodi acomodi requested a review from mithro May 24, 2021 14:25
@acomodi
Copy link
Contributor Author

acomodi commented May 24, 2021

@mithro Given that the actions check fix might take a while to land, I opted to get to use the same strategy as in tool perf to check-in generated data. Does this look good?

@mithro
Copy link
Contributor

mithro commented May 24, 2021

@acomodi All third_party code should be under the third_party directory, no exceptions....

@acomodi acomodi merged commit 39378d0 into chipsalliance:master Jun 11, 2021
@umarcor umarcor deleted the add-murax-test branch May 12, 2022 18:34
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.

3 participants