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

Expanded support for CFUs. #14

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft

Conversation

tcal-x
Copy link
Member

@tcal-x tcal-x commented Sep 30, 2021

The new Fomu variants are very hacked, so use carefully. They have removed many safety checks (alignment, etc). Also, they have a hard multiplier but no divider, so you would need to compile with "-march=rv32im -mabi=ilp32 -mno-div".

tcal-x and others added 30 commits November 4, 2020 17:54
- Now submodule ext/VexRiscv uses tip of `dev` branch
- Added ext/SpinalHDL submodule, also `dev` branch
- Modified `build.sbt` to bring up to date with current `dev` VexRiscv
- Modified `src/main/scala/vexriscv/GenCoreDefault.scala` to add a `--cfu` option, to add the CFU interface
- Added `VexRiscv_FullCfu.v` and `VexRiscv_FullCfuDebug.v` (and yamls), and updated `Makefile` to build them
- I did **not** rebuild the other .v/.yaml files

Signed-off-by: Tim Callahan <[email protected]>
Both L1 caches are changed from 4k direct-mapped to 8k 2-way associative.
The change to 2-way was required since each "way" can be 4k max.

Signed-off-by: Tim Callahan <[email protected]>
Signed-off-by: Tim Callahan <[email protected]>
Rebuilt just the CFU-enabled Vex Verilogs.

Signed-off-by: Tim Callahan <[email protected]>
Tweak Fomu variant for performance and LCs.
Require a memory and writeback stage for the CFU plugin.
Update the Fomu CPU Variant for performance.
Also increase "Slim" D$ to 4kB, I$ to 2kB.

Signed-off-by: Tim Callahan <[email protected]>
Signed-off-by: Tim Callahan <[email protected]>
Signed-off-by: Tim Callahan <[email protected]>
@enjoy-digital
Copy link
Member

@tcal-x: This create lots of variants very specific to https://github.com/google/CFU-Playground project and I'm not sure this should be integrated here where we try to provide more generic variants. This would probably be better to integrate these variants directly in the CFU project or in a fork, as is doing Betrusted project for example for the specific variants that have been created for it: https://github.com/betrusted-io/pythondata-cpu-vexriscv.

Fomu is also not a good name for a variant (I imagine it has been created to limit resource usage on iCE40) since we would also want to use it on the iCEBreaker, so we could eventually integrate this one, but would probably have to find a proper name.

cc @mithro, @kgugala

@@ -17,6 +17,16 @@ src/main/scala/vexriscv GenCoreDefault.scala
- Available in normal an -Debug, with the Debug bus exposed


## Changes in `tcal-x`'s fork
Copy link

Choose a reason for hiding this comment

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

This needs fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely.

@@ -6,10 +6,12 @@ import spinal.lib._
import spinal.lib.sim.Phase
Copy link

Choose a reason for hiding this comment

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

I don't understand why this is changing a scala file? Shouldn't this be part of the source module?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the top generator script that handles user options and then builds the VexRiscv accordingly, adding the plugins etc.. It's not part of the VexRiscv "library". It is local to the pythondata_cpu_vexriscv repo.

For example, cache line size is currently hardcoded in this file. If we want to make it an option that can be changed, we'd alter this file to add an user option and then pass the value to where the caches are instantiated.

@mithro
Copy link

mithro commented Oct 1, 2021

@enjoy-digital - I think we might need a way to provide pythondata-cpu-vexriscv-XXX packages which add more options outside the base package. Having people have their own forks with the same name as the pythondata-cpu-vexriscv package create a huge number of issues.

@@ -1,6 +1,6 @@
// Generator : SpinalHDL v1.6.0 git head : 73c8d8e2b86b45646e9d0b2e729291f2b65e6be3
// Component : VexRiscv
// Git hash : 6276bf628be9d0a58c0284dca83137b71ef29098
// Git hash : 593e180abf5ba39e7fa33d4f371646453f84496a
Copy link

Choose a reason for hiding this comment

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

we don't change anything in this file, but hash. This looks weird

@@ -1,6 +1,6 @@
// Generator : SpinalHDL v1.6.0 git head : 73c8d8e2b86b45646e9d0b2e729291f2b65e6be3
// Component : VexRiscv
// Git hash : 6276bf628be9d0a58c0284dca83137b71ef29098
// Git hash : 593e180abf5ba39e7fa33d4f371646453f84496a
Copy link

Choose a reason for hiding this comment

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

we don't change anything in this file, but hash. This looks weird

@@ -1,6 +1,6 @@
// Generator : SpinalHDL v1.6.0 git head : 73c8d8e2b86b45646e9d0b2e729291f2b65e6be3
// Component : VexRiscv
// Git hash : 6276bf628be9d0a58c0284dca83137b71ef29098
// Git hash : 593e180abf5ba39e7fa33d4f371646453f84496a
Copy link

Choose a reason for hiding this comment

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

we don't change anything in this file, but hash. This looks weird

@@ -1,34 +0,0 @@
*.class
Copy link

Choose a reason for hiding this comment

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

why do you remove gitignore?

@@ -1,6 +1,6 @@
// Generator : SpinalHDL v1.6.0 git head : 73c8d8e2b86b45646e9d0b2e729291f2b65e6be3
// Component : VexRiscv
// Git hash : 6276bf628be9d0a58c0284dca83137b71ef29098
// Git hash : 593e180abf5ba39e7fa33d4f371646453f84496a
Copy link

Choose a reason for hiding this comment

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

hash only

@@ -1,6 +1,6 @@
// Generator : SpinalHDL v1.6.0 git head : 73c8d8e2b86b45646e9d0b2e729291f2b65e6be3
// Component : VexRiscv
// Git hash : 6276bf628be9d0a58c0284dca83137b71ef29098
// Git hash : 593e180abf5ba39e7fa33d4f371646453f84496a
Copy link

Choose a reason for hiding this comment

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

again hash (there are many such updates in this PR)

@enjoy-digital
Copy link
Member

@enjoy-digital - I think we might need a way to provide pythondata-cpu-vexriscv-XXX packages which add more options outside the base package. Having people have their own forks with the same name as the pythondata-cpu-vexriscv package create a huge number of issues.

@mithro: This could be that yes, people doing specialized variants would also have to ensure they are not reusing existing names to avoid confusion (Betrusted added a Betrusted suffix for example, similarly to whas is done with CFU variants). Another option would also be to do things similarly to VexRiscv-SMP where CPU parameters are directly exposed to the user and the CPU is automatically rebuilt when the variant is not found in the cached ones, but this requires SBT and VexRiscv toolchain installed on the machine. That's what is used currently in Linux-on-LiteX-VexRiscv to avoid too much cached variants, which I'm also trying to avoid here.

@enjoy-digital
Copy link
Member

@tcal-x, @mithro: It seems the auto-build feature is already in place in CFU playground: google/CFU-Playground#305 so it would probably be better to use it and cache some CPU variants if you want to ease access to users.

@tcal-x
Copy link
Member Author

tcal-x commented Oct 4, 2021

Hello @enjoy-digital , I agree with all of your points. And thank you for keeping the change rate low in pythondata_cpu_vexriscv; it makes it easy to maintain my fork. I guess I was so fed up with the CFU Playground dependency on my personal GitHub fork, which I desperately want to remove, that I didn't think through better alternatives.

One thing I now realize is that this wouldn't be the last update -- we are still iterating on the ideal CPU for our use case, so we continue to need somewhere to commit and share experimental variants.

I've now thought through some of the alternatives.

  • I could create a new org to host the CFU-specific fork (I hadn't realized that it was so easy to create an org, but I might not understand everything that comes along with that).

  • Could the "fork" instead be a non-master branch on litex-hub/pythondata_cpu_vexriscv? I don't see that often in practice, but the README does show the possibliity of installing from a branch: pip install --user git+https://github.com/litex-hub/pythondata-cpu-vexriscv.git@<branch>

  • I could create a static directory inside of CFU-Playground that's essentially the same as the pythondata_cpu_vexriscv/pythong_cpu_vexriscv/verilog directory of my fork -- it wouldn't include the Python packaging stuff, so it couldn't be used that way; the pre-generated Verilogs would only be used directly by CFU-Playground. A user with sbt installed could generate and experiment with new variants, and commit the ones they want to share.

    • One benefit is that new CPU variants would be part of the CFU Playground commit stream.
    • But, sharing outside of CFU Playground would be more difficult.
    • Also, breaking the fork relationship with pythondata_cpu_vexriscv means that it might be necessary to manually copy in upstream changes to build.sbt and GenCoreDefault.scala.
  • Dynamic generation of variants inside of CFU Playground (simply by requesting a new variant name that encodes the desired parameterization). This would imitate to some degree what's done in pythondata_cpu_vexriscv_smp. But we could have the generation and some pre-generated common variants directly within CFU Playground. We'd need to be able to map all of the parameters from an encoded variant name -- for example, in Vex SMP, there's VexRiscvLitexSmpCluster_Cc1_Iw32Is4096Iy1_Dw32Ds4096Dy1_Ldw128_Cdma_Ood.v. In CFU Playground, our list of VexRiscv parameters is even longer and still growing, but the name would only need to have the parameters that differed from the default.

@kgugala , I've realized that there are a number of problems with the implementation of google/CFU-Playground#305 , so while parts of it might be adapted to one of the last two alternatives above, it won't go in as-is.

@tcal-x
Copy link
Member Author

tcal-x commented Oct 4, 2021

Perhaps google/CFU-Playground#309 is a better place for any continued discussion / suggestions.

@enjoy-digital
Copy link
Member

@tcal-x: My concern was mainly about the number of pre-generated variants added to the repository. A temporary approach could be to:

  1. Integrate the CFU related changes to build.sbt and GenCoreDefault.scala to avoid having different version of this.
  2. Also make sure to have the eventual CFU related changes integrated in LiteX/VexRiscv CPU wrapper.
  3. Allow dynamic generation of variants in this repository when not already pre-generated (Which also be useful for non-CFU configuration).
  4. While you are still refining the optimal CFU configurations, store the pre-generated variants in a different repository or branch. (Since pre-generated variants are large can pollute git repositories if just temporary).

For 4) if you are able in the end to limit the number of pre-generated versions you want to provide to user, I would be fine having them directly in the main branch. But while this is still experimented, we could have the pre-generated version in a branch (that we could eventually delete later) or in a CFU specific dev fork that would be the main + just a different Makefile + more pre-generated variants.

@tcal-x tcal-x marked this pull request as draft October 7, 2021 21:07
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.

5 participants