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.

Sign up for GitHub

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

Jump to bottom

Upstream features from chimera #89

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

Upstream features from chimera #89

wants to merge 20 commits into from

Conversation

sermazz
Copy link

@sermazz sermazz commented Feb 18, 2025 •
edited
Loading

  • riscv cores with TNN extension (ISA extension for ternary-weight neural networks)
  • up-to-date dmac_wrap for iDMA
  • iDMA requests to SoC do not go anymore through cluster bus, but through an additional, independent (wide) port
  • iDMA transfers to/from TCDM can now go through wide HWPEs port of TCDM

Cc: @arpansur @da-gazzi @DanielKellerM

To-do

Dependencies

Hardware

  • Solve combinational loop when iDMA connects to HWPE port of HCI (for now solved with FIFOs)
  • Parametrize the additional wide port of cluster and HCI (if using mchan we don't want the additional wide master port on the interface of the cluster, or we might want iDMA without wide transfers)

Verification

  • Rebase @DanielKellerM's iDMA verification on this branch
  • Verify with different hw configurations (multiple/different HWPEs, with/without EEC, with/without HMR, with different cores, with iDMA connected to narrow ports of HCI)
  • Merge pulp-cluster-nonfree to have proper CI testing those configurations

@sermazz sermazz force-pushed the smazzola/chimera branch 2 times, most recently from 0b4d3c1 to e329d44 Compare February 20, 2025 10:10
@FrancescoConti
Copy link
Member

As #87 was just merged, this is the next major PR for the cluster. @sermazz check if you can rebase this on master!

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
env/env.sh
@@ -6,7 +6,7 @@
ROOTD=$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")/.." && pwd)

if (hostname | grep -qE "\.ee\.ethz\.ch$") ; then
export PULP_RUNTIME_GCC_TOOLCHAIN=/usr/pack/riscv-1.0-kgf/pulp-gcc-1.0.16
export PULP_RUNTIME_GCC_TOOLCHAIN=/usr/scratch/dolcedorme/smazzola/install/pulp-tnn-gnu-7.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Is this compiler available open-source? This is important before we merge this PR

Copy link
Author

Choose a reason for hiding this comment

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

It is open-source, but as a fork of @da-gazzi.. we should merge it but I'm no expert here
https://github.com/da-gazzi/pulp-tnn-gnu-toolchain
As far as I could tell it's also outdated with respect to the currently used toolchain of the upstream PULP Cluster

Copy link
Member

Choose a reason for hiding this comment

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

Let's evoke @gtagliavini for this question :D

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
rtl/cluster_interconnect_wrap.sv
@@ -56,9 +61,9 @@ module cluster_interconnect_wrap
input logic [5:0] cluster_id_i,
XBAR_PERIPH_BUS.Slave hci_ecc_periph_slave,
hci_core_intf.target core_tcdm_slave [0 : NB_CORES-1 ],
hci_core_intf.target hwpe_tcdm_slave [0 : 0 ],
hci_core_intf.target hwpe_tcdm_slave [0 : NB_HWPE-1 ],
Copy link
Member

Choose a reason for hiding this comment

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

This will fail if NB_HWPE=0, which I think is still possible (if iDMA uses LIC ports). See pulp-platform/hci#50 for a possible better solution.

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
rtl/cluster_interconnect_wrap.sv
end
// if DMA_USE_HWPE_PORT, assign DMA interfaces to the rest of s_hwpe_intc[:]
for (genvar i=NB_HWPE; i<N_HCI_HWPE_PORTS; i++) begin : connect_dma_hwpe_intf
hci_core_assign_expand #(
Copy link
Member

Choose a reason for hiding this comment

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

This depends on pulp-platform/hci#45, which should be merged before this one is merged.

Copy link
Author

Choose a reason for hiding this comment

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

Yes there are also other dependencies that I am taking an eye on, that need to be merged before we merge this (iDMA for example)

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a check-list in the PR description to keep track!

Copy link
Author

Choose a reason for hiding this comment

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

On it!

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
rtl/cluster_interconnect_wrap.sv
.N_MEM ( NB_TCDM_BANKS ),
.IW ( TCDM_ID_WIDTH ),
.TS_BIT ( TEST_SET_BIT ),
.EXPFIFO ( 2 ),
Copy link
Member

Choose a reason for hiding this comment

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

Why the FIFO? did you have problems with timing closure?

Copy link
Author

Choose a reason for hiding this comment

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

Because there is a combinational loop.. I was not sure how to solve it

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we should check this one separately!

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
rtl/cluster_interconnect_wrap.sv
.N_MEM ( NB_TCDM_BANKS ),
.IW ( TCDM_ID_WIDTH ),
.TS_BIT ( TEST_SET_BIT ),
.EXPFIFO ( 2 ),
Copy link
Member

Choose a reason for hiding this comment

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

why the FIFO? problems with timing closure?

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
rtl/core_region.sv
@@ -232,25 +234,30 @@ import rapid_recovery_pkg::*;
assign boot_addr = boot_addr_i;
riscv_core #(
Copy link
Member

Choose a reason for hiding this comment

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

Here I believe we need to manage the two cores differently. They are all riscv_core but there are substantial differences and I fear merging all the changes is far from trivial.
We have

  • a riscv_nn that now resides in https://github.com/pulp-platform/flex-v, with the HMR branch that is referred to in the current cluster v3
  • the TNN core from @da-gazzi you target here (where is it? where did it branch out?)
  • CV32E40P, with a different history
    @alenad95 do you have suggestions on how to cut this Gordian knot?

Copy link
Author

@sermazz sermazz Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely, this is an important point.
The RI5CY (riscv-nn) used in this PR is on our gitlab: https://iis-git.ee.ethz.ch/pulpissimov2/riscv-nn/-/tree/53b053cd0be8dae25caa24bd57824e81cf87ba21

Copy link
Member

Choose a reason for hiding this comment

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

OK, it's not tragic, it's only a few commits away from the main of https://github.com/pulp-platform/flex-v/network .
@sermazz can you push TNN to a branch of flex-v? then @alenad95 i think we should have a look whether the changes can be merged (at least with the Shaheen branch), which will leave us at two flex-v branches (main + hmr) and cv32e40p.
Then we can tidy the room up a bit in a future PR with an if-generate like in PULPissimo...

FrancescoConti
FrancescoConti reviewed Feb 25, 2025
View reviewed changes
da-gazzi and others added 8 commits February 27, 2025 11:02
@da-gazzi @sermazz
hw,tb: Detach DMA from cluster bus, add wide port, test
c6cd2f5
@sermazz
hw,tb: Share HCI shallow intc between (wide) DMA and HWPEs
8c981ad
@sermazz
tb: handle non-word (64-bit bursts) aligned sections
f2edb8e 
The linker script has L1 address ORIGIN set to 0x10000004 even through
in hardware it is set to 0x10000000. However the testbench assumes 64b
alignment to initialize the L1. Thus, the data was shifted by 32b in the
simulation. While the AXI bursts are set to 64b, the misalignment needs
to be handled coming from the linker script.
@sermazz
hw: Parametrize number of EXT ports to TCDM
61649fa
@sermazz
makefile: Update nonfree CI tests with Neureka
8d32b02
@sermazz
hw: Fix combinational loop in HCI
bb28997
@sermazz
tb: Disable ECC hardware
f3218dc
@sermazz
package: Change config struct to packed type
2c36d28 
Being unnecessarily unpacked, it was not compatible with other systems (like Cheshire)
@sermazz
makefile: Bump pulp-runtime and regression_tests
67bcfb3 
New version rebased on latest Astral updates
@sermazz sermazz requested a review from arpansur February 27, 2025 10:42
@sermazz sermazz mentioned this pull request Feb 27, 2025
Draft
7 tasks
@sermazz
bender: Add obi, bump idma, fix_redundancy_cells, update Bender.lock
c83e750 
- Several sub-dependencies in the Bender.lock were not aligned with the
  Bender.yml
- obi was in the Bender.lock but not in Bender.yml
- redundancy_cells' version in Bender.yml was not supported here
@sermazz
hw: Fix idma_wrap for updated obi v0.1.3
3703186
@sermazz
makefile: Add target to generate iDMA hardware
5413e7d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrancescoConti FrancescoConti FrancescoConti left review comments

@arpansur arpansur Awaiting requested review from arpansur

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sermazz @FrancescoConti @da-gazzi