Add L1 Instruction Cache #152
Conversation
|
I haven't managed to make much progress this week, rather than leave the PR to rot. I think I'd merge this and update #143 with the enhancements I'd hope to do, and open a new PR. Probably makes code review easier anyways. |
klingaard
dismissed
their stale review
More changes were added. Need to look them over a bit.
| std::deque<InstPtr> ibuf_; | ||
|
|
||
| // Size of trace buffer (must be sized >= L1ICache bandwidth / 2B) | ||
| const uint32_t ibuf_capacity_; | ||
|
|
||
| // Fetch buffer: Holds a queue of instructions that are either | ||
| // waiting for an ICache hit response, or they're ready to be | ||
| // send to decode | ||
| std::deque<InstPtr> fetch_buffer_; |
There was a problem hiding this comment.
Consider using a sparta::Queue or sparta::Buffer here to get free pipeline collection, capacity checks, and utilization stats.
| { | ||
| const auto & inst_ptr = inst_generator_->getNextInst(my_clk_); | ||
| if (SPARTA_EXPECT_TRUE(nullptr != inst_ptr)) { | ||
| ibuf_.emplace_back(inst_ptr); |
There was a problem hiding this comment.
Wonder if you can append to the ibuf_ as blocks instead of individual instructions...
|
@danbone are you planning to add a documentation describing uarch of the modeled L1 Icache and changes to the fetch unit as a part of this PR? |
| "cpu.core*.biu.ports.out_biu_ack", | ||
| "cpu.core*.l2cache.ports.in_biu_l2cache_ack" | ||
| "cpu.core*.biu.ports.out_biu_credits", | ||
| "cpu.core*.l2cache.ports.in_biu_l2cache_credits" |
There was a problem hiding this comment.
In the planned documentation, can you please include a list of input and output ports of the Icache and what do they represent. It would be helpful to understand this part of code changes.
| // Poll on dcache_l2cache_credits_ > 0 which means | ||
| // DCache is blocking for now. Busy is set on miss, until the miss is | ||
| // resolved by the L2. | ||
| // For NB behaviour: Poll on dcache_l2cache_credits_ > 0 which means |
There was a problem hiding this comment.
If you would like to use NB as an acroynym, can you please define it somewhere first.
If this comment relates to a future change rather the current behavior, please add a TODO: before the comment, to make it explicit.
| num_insts_to_fetch_(p->num_to_fetch), | ||
| skip_nonuser_mode_(p->skip_nonuser_mode), | ||
| my_clk_(getClock()) | ||
| icache_block_shift_(sparta::utils::floor_log2(p->block_width.getValue())), | ||
| ibuf_capacity_(std::ceil(p->block_width / 2)), // buffer up instructions read from trace |
There was a problem hiding this comment.
I don't understand why we need block_width / 2 as ibuf capacity. Can you please add a comment as an explanation?
| // Capture when the simulation is stopped prematurely by the ROB i.e. hitting retire limit | ||
| node->getParent()->registerForNotification<bool, Fetch, &Fetch::onROBTerminate_>( | ||
| this, "rob_stopped_notif_channel", false /* ROB maybe not be constructed yet */); | ||
|
|
There was a problem hiding this comment.
Can you please clarify why this is needed?
| void Fetch::fetchInstruction_() | ||
| { | ||
| const uint32_t upper = std::min(credits_inst_queue_, num_insts_to_fetch_); | ||
| // Prefill the ibuf with some instructions read from the tracefile | ||
| // keeping enough capacity to group them into cache block accesses. |
There was a problem hiding this comment.
Is there a specific reason why we need to prefill instructions from the trace? Can we not read the trace after we get the response back from Icache? When we have decoupled frontend, isn't that what we would expect to do?
| void getRequestFromFetch_(const MemoryAccessInfoPtr &); | ||
| void getCreditsFromL2Cache_(const uint32_t &); | ||
| void getRespFromL2Cache_(const MemoryAccessInfoPtr &); |
There was a problem hiding this comment.
Why callbacks? Why are in ports below not enough?
|
What is the status of this PR? I see it now has conflicts, if those were fixed is this ready to go? I have a need in BPU evals. |
Adding an L1 Instruction Cache for fetch as noted in #143