#18006: Add push router support to tt-fabric. #18564

ubcheema · 2025-03-03T18:38:04Z

Adding support for push buffers in TT-Fabric routers.

nhuang-tt · 2025-03-03T19:00:46Z

How does the programming model change when you use the push method in this? Do we need need to allocate space for the headers?
Right now i need to put 48B before all my data to make space for the header

ubcheema · 2025-03-03T19:07:44Z

How does the programming model change when you use the push method in this? Do we need need to allocate space for the headers? Right now i need to put 48B before all my data to make space for the header

It is the same in this PR. You need to allocate packet header space in the data buffer.
I will add "no inline header" support soon. This will allow you to setup a header memory separately from the payload.

daminakaTT · 2025-03-03T19:31:58Z

tt_metal/fabric/hw/inc/tt_fabric.h

@@ -37,7 +40,649 @@ inline uint64_t get_timestamp() {
    return (((uint64_t)timestamp_high) << 32) | timestamp_low;
 }

-typedef struct fvc_consumer_state {
+typedef struct fvc_outbound_push_state {


How about having base struct and inherit for both push and pull. There are same variables and copy&paste. and also #ifdef around code could be removed by having big #ifdef in this file.

daminakaTT · 2025-03-03T19:33:12Z

tt_metal/fabric/impl/kernels/tt_fabric_router.cpp

+#ifdef FVC_MODE_PULL
+fvc_outbound_pull_state_t fvc_outbound_state __attribute__((aligned(16)));  // replicate for each fvc
+#else
+// fvc_inbound_push_state_t fvc_inbound_state;


daminakaTT · 2025-03-03T19:33:27Z

tt_metal/fabric/impl/kernels/tt_fabric_router.cpp

        FABRIC_ROUTER_DATA_BUF_START + (fvc_data_buf_size_words * PACKET_WORD_SIZE_BYTES / 2),
        fvc_data_buf_size_words / 2);
+#else
+    fvc_outbound_state[0].init(


can be for loop

daminakaTT · 2025-03-03T19:34:37Z

tt_metal/fabric/hw/inc/tt_fabric_api.h

+    router_addr += direction * 8;
+    // stream register to receive router buffer space available updates.
+    uint64_t xy_local_addr = get_noc_addr(0);
+    noc_inline_dw_write(router_addr, (STREAM_REG_ADDR(0, STREAM_REMOTE_DEST_BUF_SPACE_AVAILABLE_UPDATE_REG_INDEX)));
+    noc_inline_dw_write(router_addr + 4, xy_local_addr >> 32);


What are these magic number. 8 and 4

Will update comments. NOC addresses are 64-bits, 8 bytes. To calculate respective offset for a direction, its direction * 8.
line 187 is writing lower/upper 4 bytes of the 8 bytes with two noc inline dword writes.

daminakaTT · 2025-03-03T19:36:43Z

tt_metal/fabric/hw/inc/tt_fabric.h

+            // set the stream auto increment to use on remote router according to
+            // this router's direction.
+            router_push_addr =
+                (STREAM_REG_ADDR(6 + my_direction, STREAM_REMOTE_DEST_BUF_SPACE_AVAILABLE_UPDATE_REG_INDEX));


magic number 6?

tt_metal/fabric/hw/inc/tt_fabric.h

tt-aho · 2025-03-03T21:05:50Z

tests/tt_metal/tt_metal/perf_microbenchmark/routing/test_tt_fabric_sanity.cpp

@@ -1410,6 +1413,7 @@ int main(int argc, char **argv) {
    bool fixed_async_wr_notif_addr = test_args::has_command_option(input_args, "--fixed_async_wr_notif_addr");

    benchmark_mode = test_args::has_command_option(input_args, "--benchmark");
+    push_mode = test_args::has_command_option(input_args, "--push_router");


Can we add tests for the push model to CI?
We have:
Post Commit on N300: tests/scripts/run_cpp_fabric_tests.sh
T3k unit tests: tests/scripts/t3000/run_t3000_unit_tests.sh
TG unit tests: tests/scripts/tg/run_tg_unit_tests.sh

added tests to CI.

tt_metal/fabric/hw/inc/routing_table.h

tt_metal/fabric/hw/inc/tt_fabric_api.h

abhullar-tt

made some comments to ease uplift to BH

tt_metal/fabric/hw/inc/tt_fabric.h

tt_metal/fabric/hw/inc/tt_fabric_api.h

daminakaTT · 2025-03-03T23:19:33Z

tt_metal/fabric/hw/inc/tt_fabric.h

+        packet_word_0 = (volatile uint32_t*)get_local_buffer_read_addr();
+        packet_word_1 =
+            reinterpret_cast<tt_l1_ptr uint32_t*>(buffer_start + (out_rdptr_inc<fvc_mode>(1) * PACKET_WORD_SIZE_BYTES));
+        packet_word_2 =
+            reinterpret_cast<tt_l1_ptr uint32_t*>(buffer_start + (out_rdptr_inc<fvc_mode>(2) * PACKET_WORD_SIZE_BYTES));


What are these 0,1,2 for?

These are pointers to first 3 packet words which point to packet header. It accounts for the case where the header might be wrapping at the end of the buffer.

daminakaTT · 2025-03-03T23:20:24Z

tt_metal/fabric/hw/inc/tt_fabric.h

+        uint32_t temp = packet_word_0[0];
+        sender_buffer_index = temp >> 30;
+        temp &= 0x3FFFFFFF;
+        packet_words_remaining = (temp + PACKET_WORD_SIZE_BYTES - 1) >> 4;


this 'temp' looks having more meaning than temporal value. What is this?

daminakaTT · 2025-03-03T23:23:10Z

tt_metal/fabric/hw/inc/tt_fabric.h

+    }
+
+    uint32_t get_next_hop_router_noc_xy() {
+        uint32_t dst_mesh_id = packet_word_0[1] & 0xFFFF;


What is this indexing and masking?
Looks like this packet_word_* is accessing header values? If so I want more explicit accessor. getter or casting

daminakaTT · 2025-03-03T23:48:47Z

tt_metal/fabric/impl/kernels/tt_fabric_router.cpp

+            loop_count = 0;
+        } else if (
+            *fvc_outbound_state[next_outbound_buffer].noc_word_credits ||
+            *fvc_outbound_state[next_outbound_buffer].sender_words_cleared) {


Use getter? get_num_words_cleared
or wrap these two check in one method?

daminakaTT · 2025-03-03T23:52:35Z

tt_metal/fabric/impl/kernels/tt_fabric_router.cpp

+        } else if (
+            *fvc_outbound_state[next_outbound_buffer].noc_word_credits ||
+            *fvc_outbound_state[next_outbound_buffer].sender_words_cleared) {
+            curr_outbound_buffer = next_outbound_buffer;
+            next_outbound_buffer = (next_outbound_buffer + 1) & 0x3;
+        } else {
+            next_outbound_buffer = (next_outbound_buffer + 1) & 0x3;
+        }


else { if (*fvc_outbound_state[next_outbound_buffer].noc_word_credits || *fvc_outbound_state[next_outbound_buffer].sender_words_cleared) { curr_outbound_buffer = next_outbound_buffer; } next_outbound_buffer = (next_outbound_buffer + 1) & 0x3; }

daminakaTT · 2025-03-03T23:57:08Z

tt_metal/fabric/hw/inc/tt_fabric.h

+        }
+    }
+
+    FORCE_INLINE void get_words_pushed() {


getter should return something.

daminakaTT · 2025-03-04T00:59:48Z

tt_metal/fabric/hw/inc/tt_fabric.h

+        if constexpr (fvc_mode == FVC_MODE_ROUTER) {
+            tt::tt_fabric::chan_id_t my_chan = routing_table->intra_mesh_table.dest_entry[device_id];
+            uint32_t my_direction;
+            if (routing_table->port_direction.directions[eth_chan_directions::EAST] == my_chan) {


Looks like there are duplicate code. How about having function

daminakaTT · 2025-03-04T01:00:06Z

tt_metal/fabric/hw/inc/tt_fabric.h

+            next_port = routing_table->intra_mesh_table.dest_entry[dst_dev_id];
+        }
+
+        if (routing_table->port_direction.directions[eth_chan_directions::EAST] == next_port) {


daminakaTT · 2025-03-04T01:01:09Z

tt_metal/fabric/hw/inc/tt_fabric_api.h

+        next_port = routing_table[routing_plane].intra_mesh_table.dest_entry[dst_dev_id];
+    }
+
+    if (routing_table[routing_plane].port_direction.directions[eth_chan_directions::EAST] == next_port) {


nhuang-tt · 2025-03-04T02:24:21Z

tt_metal/fabric/hw/inc/tt_fabric.h

+    inline void init(uint32_t buffer_id, uint32_t data_buf_start, uint32_t data_buf_size_words) {
+        uint32_t words = sizeof(fvc_outbound_push_state) / 4;
+        uint32_t* ptr = (uint32_t*)this;
+        for (uint32_t i = 0; i < words; i++) {


Can be removed? This is zeroing the struct but we are reassigning what was just set to zero below.

Its initially clearing all the structure data to 0.
Later members are initialized to respective values.
Not all the members end up with non-zero values.

nhuang-tt · 2025-03-04T02:25:27Z

tt_metal/fabric/hw/inc/tt_fabric.h

+    inline void init(uint32_t data_buf_start, uint32_t data_buf_size_words) {
+        uint32_t words = sizeof(fvc_inbound_push_state) / 4;
+        uint32_t* ptr = (uint32_t*)this;
+        for (uint32_t i = 0; i < words; i++) {


Same as above

not all structure data members get set later in the init code.

nhuang-tt · 2025-03-04T02:28:48Z

tt_metal/fabric/hw/inc/tt_fabric.h

+                    continue;
+                }
+                uint32_t forwarding_channel = routing_table->port_direction.directions[i];
+                if (forwarding_channel == INVALID_DIRECTION) {


Do we need to check for this INVALID_DIRECTION?
If the direction was truly invalid as the name implies, a corrupt value like 123, wouldn't be caught because the forwarding channel is a uint32_t and it won't match to any of the enum values.
If INVALID_DIRECTION means to skip this direction then consider renaming it.

Not part of this commit. So we can update this later.
control plane sets the port directions when its initialized.
In this code, we are only consuming the entries in the routing table.

Some of the initializations will eventually move to host code and removed from kernel.

daminakaTT · 2025-03-04T07:49:46Z

tt_metal/fabric/hw/inc/tt_fabric.h

+    template <uint8_t fvc_mode = FVC_MODE_ROUTER>
+    void register_with_routers(uint32_t device_id, uint32_t mesh_id = 0) {
+        if constexpr (fvc_mode == FVC_MODE_ROUTER) {
+            tt::tt_fabric::chan_id_t my_chan = routing_table->intra_mesh_table.dest_entry[device_id];


Where & how is this routing_table initialized?
globally declared and defined by each includer?

daminakaTT · 2025-03-04T07:51:29Z

tt_metal/fabric/hw/inc/tt_fabric.h

+                STREAM_ID_NOC_WORDS_RECEIVED + my_direction, STREAM_REMOTE_DEST_BUF_SPACE_AVAILABLE_UPDATE_REG_INDEX));
+            remote_buffer_start = FABRIC_ROUTER_DATA_BUF_START + my_direction * FABRIC_ROUTER_OUTBOUND_BUF_SIZE;
+
+            for (uint32_t i = 0; i < eth_chan_directions::COUNT; i++) {


i = eth_chan_directions::EAST?

oh, yeah, meant to to that.

daminakaTT · 2025-03-04T07:57:51Z

tt_metal/fabric/hw/inc/tt_fabric.h

+        if (dst_mesh_id != routing_table->my_mesh_id) {
+            uint32_t next_port = routing_table->inter_mesh_table.dest_entry[dst_mesh_id];
+            remote_wrptr_direction = port_direction_table[next_port];
+            return eth_chan_to_noc_xy[noc_index][next_port];
+        } else {
+            uint32_t dst_device_id = packet_word_0[1] >> 16;
+            uint32_t next_port = routing_table->intra_mesh_table.dest_entry[dst_device_id];
+            remote_wrptr_direction = port_direction_table[next_port];
+            return eth_chan_to_noc_xy[noc_index][next_port];
+        }


uint32_t next_port; if (dst_mesh_id != routing_table->my_mesh_id) { next_port = routing_table->inter_mesh_table.dest_entry[dst_mesh_id]; } else { uint32_t dst_device_id = packet_word_0[1] >> 16; next_port = routing_table->intra_mesh_table.dest_entry[dst_device_id]; } remote_wrptr_direction = port_direction_table[next_port]; return eth_chan_to_noc_xy[noc_index][next_port];

These if else looks really common. How about having private inlined get_next_port or something?

No, this code hurts performance.
It is nice to look at, but not good for performance.
local scope variable and return allow the code to not maintain register variables across the if/else block.

aagarwalTT · 2025-03-04T15:02:28Z

tt_metal/fabric/hw/inc/tt_fabric_api.h

+}
+
+template <AsyncWriteMode mode = AsyncWriteMode::ALL>
+inline void fabric_async_write(


Should we add the client_interface type to the template as well so that we have just one definition of the async write API?

aagarwalTT · 2025-03-04T15:07:03Z

tt_metal/fabric/hw/inc/tt_fabric_api.h

+    client_interface->buffer_size = FABRIC_ROUTER_OUTBOUND_BUF_SIZE / PACKET_WORD_SIZE_BYTES;
+    client_interface->wr_ptr = 0;
+    client_interface->buffer_start = FABRIC_ROUTER_DATA_BUF_START + direction * FABRIC_ROUTER_OUTBOUND_BUF_SIZE;
+    client_interface->router_push_addr = (STREAM_REG_ADDR(


Maybe not in this PR, but should we add all the STREAM REG handling in a separate utility file? If we have all the handling in a single place we could also keep a track of what registers are in use.

Yeah, we can have it in a separate stream register utility file in a different PR.

ubcheema · 2025-03-04T16:40:40Z

Guys, In general, if there are any hazards or missing functionality, please comment on that.
Don't use this PR to learn about the code.
I am not entertaining code beautification suggestions in this PR.
We need to get this feature pushed to main.
It is not in its final form anyway. So any code rearrangement at this point is not very beneficial.
As more functionality gets added, code will morph, and I will certainly incorporate code rearrangement suggestions.

tests/scripts/t3000/run_t3000_unit_tests.sh

aliuTT

These new tests should work if you remove TT_METAL_SLOW_DISPATCH_MODE=1 and use fast dispatch?

ubcheema · 2025-03-06T22:45:13Z

These new tests should work if you remove TT_METAL_SLOW_DISPATCH_MODE=1 and use fast dispatch?

I'll check that.

ubcheema · 2025-03-07T17:09:20Z

Ran successful 1000 run loop for TT_METAL_SLOW_DISPATCH_MODE=1 ./build/test/tt_metal/perf_microbenchmark/routing/test_tt_fabric_sanity_wormhole_b0 --fabric_command 1 --board_type t3k --data_kb_per_tx 10 --push_router --metal_fabric_init_level 1

https://github.com/tenstorrent/tt-metal/actions/runs/13724561170

Use get_noc_addr_helper( ) when making NOC address. Roll sender buffer space register address in to a loop. Add tests to CI Fix bug where ethernet receiver was returning extra credits to ethernet sender. Hook up device init for push router.

ubcheema requested review from aliuTT, aagarwalTT, abhullar-tt, pgkeller, tt-aho, tt-dma, tt-asaigal, cfjchu and omilyutin-tt as code owners March 3, 2025 18:38

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from 6394ef9 to 865a5cd Compare March 3, 2025 19:00

daminakaTT reviewed Mar 3, 2025

View reviewed changes

tt_metal/fabric/hw/inc/tt_fabric.h Outdated Show resolved Hide resolved

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch 2 times, most recently from df3fff8 to 532011d Compare March 3, 2025 20:29

tt-aho reviewed Mar 3, 2025

View reviewed changes

abhullar-tt reviewed Mar 3, 2025

View reviewed changes

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from 532011d to 691bada Compare March 3, 2025 22:20

daminakaTT reviewed Mar 3, 2025

View reviewed changes

tt_metal/fabric/hw/inc/tt_fabric.h

}

}

FORCE_INLINE void get_words_pushed() {

Copy link

Contributor

daminakaTT Mar 3, 2025

Choose a reason for hiding this comment

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

getter should return something.

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from e295ef9 to 2cc6974 Compare March 4, 2025 00:03

daminakaTT reviewed Mar 4, 2025

View reviewed changes

ubcheema requested a review from a team as a code owner March 4, 2025 01:54

nhuang-tt reviewed Mar 4, 2025

View reviewed changes

daminakaTT reviewed Mar 4, 2025

View reviewed changes

aagarwalTT reviewed Mar 4, 2025

View reviewed changes

aagarwalTT approved these changes Mar 4, 2025

View reviewed changes

tt-aho approved these changes Mar 4, 2025

View reviewed changes

abhullar-tt approved these changes Mar 4, 2025

View reviewed changes

nhuang-tt self-requested a review March 5, 2025 17:49

tt-rkim reviewed Mar 5, 2025

View reviewed changes

tests/scripts/t3000/run_t3000_unit_tests.sh Show resolved Hide resolved

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch 2 times, most recently from 4dd37e4 to e5cd1af Compare March 6, 2025 20:22

tt-asaigal approved these changes Mar 6, 2025

View reviewed changes

aliuTT approved these changes Mar 6, 2025

View reviewed changes

tt-rkim approved these changes Mar 7, 2025

View reviewed changes

ubcheema requested a review from davorchap as a code owner March 7, 2025 14:43

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from 7a59971 to d8ec54a Compare March 7, 2025 16:25

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from d8ec54a to c85e0fd Compare March 7, 2025 17:52

#18006: Add push router support to tt-fabric.

192720c

Use get_noc_addr_helper( ) when making NOC address. Roll sender buffer space register address in to a loop. Add tests to CI Fix bug where ethernet receiver was returning extra credits to ethernet sender. Hook up device init for push router.

ubcheema force-pushed the ucheema/tt-fabric-push-router-support branch from c85e0fd to 192720c Compare March 7, 2025 17:56

ubcheema merged commit 0a995b8 into main Mar 7, 2025
34 checks passed

ubcheema deleted the ucheema/tt-fabric-push-router-support branch March 7, 2025 18:36

#18006: Add push router support to tt-fabric. #18564

#18006: Add push router support to tt-fabric. #18564

Conversation

ubcheema commented Mar 3, 2025

nhuang-tt commented Mar 3, 2025

ubcheema commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhullar-tt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daminakaTT Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ubcheema commented Mar 4, 2025

aliuTT left a comment

Choose a reason for hiding this comment

ubcheema commented Mar 6, 2025

ubcheema commented Mar 7, 2025

daminakaTT Mar 4, 2025 •

edited

Loading