-
Notifications
You must be signed in to change notification settings - Fork 13
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
[P2] Installation and Data Preloading Issue #11: new data type #68
base: master
Are you sure you want to change the base?
Conversation
…e mk_cpu_pkt to support both data and ctrl type from cpu in one packet
…e mk_cpu_pkt to support both data and ctrl type from cpu in one packet
controller/ControllerRTL.py
Outdated
s.recv_from_cpu_ctrl_pkt = RecvIfcRTL(CtrlPktType) | ||
s.send_to_ctrl_ring_ctrl_pkt = SendIfcRTL(CtrlPktType) | ||
s.recv_from_cpu_pkt = RecvIfcRTL(CpuPktType) | ||
s.send_to_ctrl_ring_pkt = SendIfcRTL(CpuPktType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to send_to_intra_cgra_pkt
?
controller/ControllerRTL.py
Outdated
s.recv_from_cpu_ctrl_pkt = RecvIfcRTL(CtrlPktType) | ||
s.send_to_ctrl_ring_ctrl_pkt = SendIfcRTL(CtrlPktType) | ||
s.recv_from_cpu_pkt = RecvIfcRTL(CpuPktType) | ||
s.send_to_ctrl_ring_pkt = SendIfcRTL(CpuPktType) | ||
|
||
# Request from/to tiles. | ||
s.recv_from_tile_load_request_pkt = RecvIfcRTL(NocPktType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to recv_from_local_cgra_load_request_pkt
?
controller/ControllerRTL.py
Outdated
@@ -102,6 +102,7 @@ def construct(s, ControllerIdType, CmdType, CtrlPktType, NocPktType, | |||
assert addr2controller_vector[addr_base] == -1, f"address range [{begin_addr}, {end_addr}] overlaps with others." | |||
addr2controller_vector[addr_base] = ControllerIdType(src_controller_id) | |||
|
|||
# What does this do? Connect itself? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to model/construct a look-up table (i.e., a map, a bunch of wires in HW) at design time to memorize the relationship between addr_base
and controller_id
.
Theoretically, this addr2controller_vector
should also be delivered via cpu_pkt
, and the look-up table should be updated accordingly. Can you please help create/file an issue for this?
lib/messages.py
Outdated
def mk_cpu_pkt(datatype_id, | ||
# DataType | ||
payload_nbits=16, predicate_nbits=1, bypass_nbits=1, | ||
# CtrlPktType | ||
nrouters = 4, ctrl_actions = 8, ctrl_mem_size = 4, ctrl_operations = 7, ctrl_fu_inports = 4, ctrl_fu_outports = 4, ctrl_tile_inports = 5, ctrl_tile_outports = 5, | ||
prefix="CPUPkt"): | ||
|
||
if datatype_id == 0: | ||
return mk_data(payload_nbits, predicate_nbits, bypass_nbits) | ||
else: | ||
return mk_ring_across_tiles_pkt(nrouters, ctrl_actions, ctrl_mem_size, ctrl_operations, ctrl_fu_inports, ctrl_fu_outports, ctrl_tile_inports, ctrl_tile_outports) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly same question as @yyan7223 asked: #69 (comment)
Instead of if/else here, I think we need to:
- Rename
mk_ring_across_tiles_pkt
tomk_intra_cgra_pkt
. - Extend
mk_ring_across_tiles_pkt
to havecontroller_id
to make the pkt be able to traverse on the inter-cgra NoC for ctrl/const/data delivery @yyan7223.- Existing
id
inmk_ring_across_tiles_pkt
is thetile_id
, which is not enough to representcontroller_id
(orcgra_id
).
- Existing
- Extend
mk_ring_across_tiles_pkt
to havedata
anddata_addr
for data memory write/initialization/over-write @yyan7223.- @yuqisun your const data can re-use this
data
field, as the const data and data memory's data should be the same bit-width/format.
- @yuqisun your const data can re-use this
- Add another two types of CMD_XXX:
CMD_CONST
: to indicate the pkt is for updating yourConstMemRTL
.CMD_CONST_CLEAR
: to indicate the pkt is for updating yourConstMemRTL
fromwr_cur = 0
, i.e., explicitly tellingwr_cur
go back to 0.- I am re-thinking about our discussion on whether we need to allow
wr_cur
goes back to 0. If we allow the 9th const data update the address 0 item (i.e., allowingwr_cur
go back to 0 when size/limit is 8), we don't need thisCMD_CONST_CLEAR
to explicitly go back to 0. User should make sure reader is slower than writer, and make sure write less than size number of data. We can sync on this.
- I am re-thinking about our discussion on whether we need to allow
Your current implementation (if/else) would force the controller only accept either mk_data
type or mk_ring_across_tiles_pkt
type at design/model time, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming mk_ring_across_tiles_pkt
to mk_preloading_cgra_pkt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pkt could be not only for preloading, it could be some dynamically generated pkt from CPU to guide some runtime behavior. So preloading
sounds too narrow~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, then I change it back
controller/ControllerRTL.py
Outdated
s.recv_from_cpu_ctrl_pkt //= s.recv_ctrl_pkt_queue.recv | ||
s.recv_ctrl_pkt_queue.send //= s.send_to_ctrl_ring_ctrl_pkt | ||
s.recv_from_cpu_pkt //= s.recv_pkt_queue.recv | ||
s.recv_pkt_queue.send //= s.send_to_ctrl_ring_pkt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send_to_ctrl_ring_pkt
-> send_to_intra_cgra_pkt
.
systolic/CgraSystolicArrayRTL.py
Outdated
|
||
# Connects ring with each control memory. | ||
for i in range(s.num_tiles): | ||
s.ctrl_ring.send[i] //= s.tile[i].recv_ctrl_pkt | ||
|
||
s.ctrl_ring.recv[0] //= s.controller.send_to_ctrl_ring_ctrl_pkt | ||
s.ctrl_ring.recv[0] //= s.controller.send_to_ctrl_ring_pkt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.controller.send_to_ctrl_ring_pkt
-> s.controller.send_to_intra_cgra_pkt
.
msg from CPU is sent into the recv_pkt_queue in tile's ctrl_memory VectorCGRA/mem/ctrl/CtrlMemDynamicRTL.py Line 53 in 7222dae
You are right, instead blindly connect it to tile's s.ctrl_mem.recv_pkt, we should connect the msg to both
|
…ables to more meaningful
Hi Yuqi, is there still any blocker for this? |
Thanks Cheng, I'm working on TileRTL, concern with it may impact others, will raise if any specific question. |
Hi Yuai, VectorCGRA/tile/test/TileRTL_test.py Lines 141 to 143 in 7222dae
On the other hand, VectorCGRA/tile/test/TileRTL_test.py Lines 146 to 152 in 7222dae
ordering is here: Lines 214 to 238 in dbe6b94
|
Good question btw! |
Thanks, just found it 👍 |
Hi @tancheng ,
I added a new type
mk_cpu_pkt
inmessages.py
to support bothDataType
andCtrlPktType
, could you help review if I'm on the right way? This may impact many other RTLs as the ifc name changed.And looks msg from CPU is not yet used in Tile right?
Thanks,