Track PHI first state per control slot#298
Conversation
|
Thanks @guosran, this is the only PR that I understand what it is trying to solve. Can we provide a concrete example in the description about why we need this fix? What issue it would encounter if we don't fix it? |
|
Well, the Drop unrelated PHI PR noise actually adds MemUnit fix in this PR. Can we avoid that? |
3548569 to
2d9163b
Compare
Concrete example: two different control-memory slots can both execute PHI operations. Before this fix, Without the fix, a later PHI slot may choose the loop-carried input when it should still choose the initial/const input, or vice versa. This can corrupt the value entering the loop body. The fix tracks first-state per control slot, so each PHI instruction has independent first-iteration state. |
The unrelated MemUnit change has been removed from this PR. |
2d9163b to
8c6da63
Compare
|
Concrete example: first iteration: later iterations: |
yyan7223
left a comment
There was a problem hiding this comment.
The proposed Track PHI first state per cotrol slot has already been implemented in the initial code, I don't understand what is this PR trying to resolve. Most of code changes are just modifying the signal name, e.g., renaming s.first[s.ctrl_addr_inport] to s.cur_first.
Besides, I agree with that OPT_PHI_START selects the value from port 0 for the first iteration, but I remember it should select the ports value with predicate = 1 in the following iterations just like OPT_PHI, rather than just select the value from port 1 regardless of its predicate. @tancheng WDYT?
| s.send_out[0].msg.predicate @= 0 | ||
| s.recv_all_val @= ((s.first[s.ctrl_addr_inport] & s.recv_in[s.in0_idx].val) | \ | ||
| (~s.first[s.ctrl_addr_inport] & s.recv_in[s.in0_idx].val & s.recv_in[s.in1_idx].val)) | ||
| s.send_out[0].msg.predicate @= s.recv_in[s.in1_idx].msg.predicate & \ |
There was a problem hiding this comment.
What if the loop-carried input comes from s.recv_in[s.in0_idx]? Will that happen for OPT_PHI_START?
Sorry I forget the definition of PHI_START. What's the difference between PHI_START, PHI_CONST, and PHI?
| s.send_to_ctrl_mem.val @= 0 | ||
| s.send_to_ctrl_mem.msg @= s.CgraPayloadType(0, 0, 0, 0, 0) | ||
| s.recv_from_ctrl_mem.rdy @= 0 | ||
| s.cur_first @= s.first[s.ctrl_addr_inport] |
There was a problem hiding this comment.
I think directly using s.phi_executed_first_time[s.ctrl_addr_inport] is more straightforward.
Summary
Addresses #292 with a minimized PHI fix. This PR only updates
PhiRTL.pyand its focused test.Changes
No MemUnit changes, markdown files, new ports, or new interfaces are included.