Conversation
powerjg
left a comment
powerjg
left a comment
There was a problem hiding this comment.
One thing that's missing is updates to numOutstandingMemReqs. Right now, this is going to go below 0 (we should probably add an assert on (new) line 320.
Right now, there is just one memory request added for a split packet, but we are subtracting twice. We should either only subtract when we receive both packets or we should add twice. I think the latter would be better.
|
|
||
| return pkt; | ||
| } | ||
| PacketPtr split_pkt = NULL; |
There was a problem hiding this comment.
| PacketPtr split_pkt = NULL; | |
| PacketPtr split_pkt = nullptr; |
| PacketPtr pkt, split_pkt; | ||
|
|
||
| std::tie(pkt, split_pkt) = getPacket(mem_ref); |
There was a problem hiding this comment.
| PacketPtr pkt, split_pkt; | |
| std::tie(pkt, split_pkt) = getPacket(mem_ref); | |
| auto [pkt, split_pkt] = getPacket(mem_ref); |
| } | ||
| } | ||
|
|
||
| return std::make_pair(pkt, split_pkt); |
There was a problem hiding this comment.
I think the following works in modern C++.
| return std::make_pair(pkt, split_pkt); | |
| return {pkt, split_pkt}; |
| } | ||
|
|
||
| PacketPtr | ||
| std::pair<PacketPtr, PacketPtr> |
There was a problem hiding this comment.
I think this is right, but you should double check me :).
| std::pair<PacketPtr, PacketPtr> | |
| std::tuple<PacketPtr, PacketPtr> |
| if (stats.memStallStart == 0) { | ||
| stats.memStallStart = curTick(); | ||
| } | ||
| numOutstandingMemReqs++; |
There was a problem hiding this comment.
Why do we increment the outstanding requests when the packet is failed?
There was a problem hiding this comment.
sorry, I pushed the wrong version earlier
| stats.memStallStart = curTick(); | ||
| } | ||
| delete pkt; | ||
| numOutstandingMemReqs++; |
There was a problem hiding this comment.
Again, why increcement when it fails?
| // we should not be sending the first pkt again if | ||
| // we are retrying only on the second part of the | ||
| // previously stalled request |
There was a problem hiding this comment.
This comment is for the other side of the branch, not this side.
There was a problem hiding this comment.
If I have understood correctly, the place of the comment should be changed?
There was a problem hiding this comment.
Added/fixed comments to hopefully make things better and not worse
| // Also, the assumption is that we cannot start a | ||
| // new memory request in parallel. |
There was a problem hiding this comment.
I'm not sure how to interpret this statement. Otherwise, it looks good.
2 participants
Has not been extensively tested yet.