multiple hwpe support in parallel by arpansur · Pull Request #45 · pulp-platform/hci

arpansur · 2024-07-22T16:48:37Z

I created and integrated an arbiter tree using the hci_arbiter to support multiple hwpes arbitration for wide port.

…in parallel

…urces There could be scenarios where aligned as well as misaligned sources multiplexed through a single source. The data received from the HCI is always one word more. If there are strictly aligned sources for example weight in Neureka[TP_IN=32], then they can benefit from the additional bandwidth[TP_IN*9=288b]. Thus, in the aligned case we can provide the whole DATA_WIDTH data to the downstream and it is upto the the downstream sink to slice data according to its requirement.

Some HWPE might depend on the r_valid for the read response whereas don't care for the write and they can support multiple outstanding instructions. So a r_valid generated for a write coming after multiple cycles could behave like a r_valid for a read response(read request follows a write request). Even though the TCDM responds the data after a single cycle latency, any FIFO in the request path can cause the r_valid to arrive a few cycles later even though the handshake is done(writing data to the FIFO). For selected HWPEs we can filter the r_valid for the write requests using FILTER_WRITE_R_VALID parameter exposed to the top.

fix arbiter tree to handle any number of wide ports

FrancescoConti · 2024-07-22T20:11:47Z

I will give you a detailed review, but in the meantime one important question: are you sure that the multi-dimensional arrays of interfaces are synthesizable? I strongly suspect they are not.

FrancescoConti

Mostly very minor comments. But the N-dimensional interface array are not convincing.

FrancescoConti · 2024-07-22T20:19:47Z

rtl/common/hci_helpers.svh

  )
 `define HCI_INTF(__name, __clk)                `HCI_INTF_EXPLICIT_PARAM(__name, __clk,          `HCI_SIZE_PARAM(__name))
 `define HCI_INTF_ARRAY(__name, __clk, __range) `HCI_INTF_EXPLICIT_PARAM(__name[__range], __clk, `HCI_SIZE_PARAM(__name))
+`define HCI_INTF_2D_ARRAY(__name, __clk, __range2D, __range1D) `HCI_INTF_EXPLICIT_PARAM(__name[__range2D][__range1D], __clk, `HCI_SIZE_PARAM(__name))


See my comment on 2d, 3d interfaces: I am scared by these...

FrancescoConti · 2024-07-22T20:20:14Z

rtl/core/hci_core_assign_expand.sv

+
+module hci_core_assign_expand
+  import hwpe_stream_package::*;
+    #(


same alignment as module

It seems like github aligns TAB differently. I aligned them too!

rtl/core/hci_core_assign_expand.sv

FrancescoConti · 2024-07-22T20:21:30Z

rtl/core/hci_core_r_id_filter.sv

 #(
-  parameter hci_size_parameter_t `HCI_SIZE_PARAM(tcdm_target) = '0
+  parameter hci_size_parameter_t `HCI_SIZE_PARAM(tcdm_target) = '0,
+parameter int unsigned N_OUTSTANDING = 2,


fix alignment on these lines, remove all tabs

FrancescoConti · 2024-07-22T20:22:05Z

rtl/core/hci_core_r_id_filter.sv

+  end else begin
+    logic [IW-1:0] id_q;
+    assign target_r_id = id_q;
+assign tcdm_target.gnt        = tcdm_initiator.gnt;


fix alignment

FrancescoConti · 2024-07-22T20:24:51Z

rtl/hci_interconnect.sv

-        .out     ( hwpe_mem )
+      for(genvar ii=0; ii<N_HWPE; ii++) begin : hwpe_req2mem
+
+          hci_router #(


fix all alignments in this block

FrancescoConti · 2024-07-22T20:25:49Z

rtl/interco/hci_arbiter_tree.sv

+  input  logic                   clear_i,
+  input  hci_interconnect_ctrl_t ctrl_i,
+
+  hci_core_intf.target    in    			   [0:NB_REQUESTS-1][0:NB_CHAN-1],


same as above. one possible alternative is a single [0:NB_REQUESTS*NB_CHAN-1] array... requires a bit more boilerplate though.

FrancescoConti · 2024-07-22T20:27:05Z

rtl/interco/hci_arbiter_tree.sv

+							.rst_ni  ( rst_ni              ),
+							.clear_i ( clear_i             ),
+							.ctrl_i  ( ctrl_i              ),
+							.in_high ( in[ii] 			   ),


fix alignments

FrancescoConti · 2024-07-22T20:28:38Z

rtl/interco/hci_router.sv

    .NB_IN_CHAN  ( NB_IN_CHAN  ),
-    .NB_OUT_CHAN ( NB_OUT_CHAN )
+    .NB_OUT_CHAN ( NB_OUT_CHAN ),
+    .FILTER_WRITE_R_VALID(FILTER_WRITE_R_VALID)


FrancescoConti · 2024-07-22T20:29:51Z

rtl/core/hci_core_r_id_filter.sv

+    assign tcdm_target.gnt = tcdm_initiator.gnt & !(fifo_full);
+    assign tcdm_initiator.req = tcdm_target.req & !(fifo_full);
+
+   fifo_v3 #(


i am not a huge fan of depending on common_cells... but ok.

2D interface arrays are not synthesizable at the moment. Thus, 2D interfaces are flattened to 1D arrays. Additionally the alignements are fixed

arpansur · 2024-07-23T19:09:36Z

I will give you a detailed review, but in the meantime one important question: are you sure that the multi-dimensional arrays of interfaces are synthesizable? I strongly suspect they are not.

Yes, you are right! I removed the reliance on the 2D interfaces

sermazz

I rebased this branch on top of the latest modifications to the HCI done for Astral (https://github.com/pulp-platform/hci/tree/lg/ecc_rebase_v2.1.1), so that I could use it in our Cluster pv2.
Please chek: https://github.com/pulp-platform/hci/tree/smazzola/chimera
Can we change the origin branch of this PR to smazzola/chimera? Or I open another PR and we close this? @FrancescoConti @arpansur

FrancescoConti · 2025-02-27T11:20:30Z

I am not sure we can change the origin (we can change the base from the PR interface), so I'd say let's open a new PR and then we close this one mentioning the new PR.

FrancescoConti · 2025-03-07T21:44:17Z

As discussed I opened #53, which supersedes this PR

da-gazzi and others added 11 commits July 8, 2024 16:52

[WIP] add hci_core_assign_expand module, enable multiple HWPEs

e3c9cdf

hci_interconnect.sv: fix ID widths

8604c84

modify hci_core_id_filter so that it supports r_valid delay

c717c63

[cosmetic] modified the code with more meaningful name and comments

082b5fb

[update] added arbiter tree to enable multiple hwpe accessing memory …

ed2170f

…in parallel

[update] added 2D and 3D array definitions to the hci interface

53bf37a

[update] instantiated arbiter_tree to arbitrate between multiple hwpes

62d1f38

[fix] bug fix on handling the requests through arbiter tree

ca3c8eb

[fix] fix arbiter tree

8390914

fix arbiter tree to handle any number of wide ports

arpansur requested a review from FrancescoConti July 22, 2024 16:48

FrancescoConti requested changes Jul 22, 2024

View reviewed changes

[update] removed reliance on 2D interface arrays

ef325e5

2D interface arrays are not synthesizable at the moment. Thus, 2D interfaces are flattened to 1D arrays. Additionally the alignements are fixed

arpansur force-pushed the prasadar/multi-hwpe-support branch 2 times, most recently from 70544a1 to e54ae19 Compare July 23, 2024 15:16

[cosmetic] white space

df0540a

arpansur force-pushed the prasadar/multi-hwpe-support branch from e54ae19 to df0540a Compare July 23, 2024 15:29

FrancescoConti mentioned this pull request Feb 25, 2025

Upstream features from chimera pulp-platform/pulp_cluster#89

Closed

14 tasks

sermazz self-requested a review February 27, 2025 10:25

sermazz requested changes Feb 27, 2025

View reviewed changes

FrancescoConti mentioned this pull request Mar 7, 2025

Multiple HWPE support + ECC #53

Merged

FrancescoConti closed this Mar 7, 2025

Conversation

arpansur commented Jul 22, 2024

Uh oh!

FrancescoConti commented Jul 22, 2024

Uh oh!

FrancescoConti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arpansur commented Jul 23, 2024

Uh oh!

sermazz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoConti commented Feb 27, 2025

Uh oh!

FrancescoConti commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

sermazz left a comment •

edited

Loading