Add L1 Instruction Cache #152

danbone · 2024-02-08T13:58:50Z

Adding an L1 Instruction Cache for fetch as noted in #143

Updated fetch unit so that it requests a block of instructions per cycle from the instruction cache. The block is formed based on the instruction's program counters.
Tidied up some coding style within the L2Cache/MSS.
Added tests for new instruction cache unit.
Changed MSS credit interfaces to be more like those used within the core.

klingaard

LGTM. Appreciate your changing code to match the Code Style guide.

Signed-off-by: danbone <[email protected]>

danbone · 2024-02-16T12:58:15Z

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.

…_icache

core/DCache.cpp

…/riscv-perf-model into danbone/issue_143_icache

More changes were added. Need to look them over a bit.

klingaard · 2024-02-26T17:18:21Z

core/Fetch.hpp

+        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_;


Consider using a sparta::Queue or sparta::Buffer here to get free pipeline collection, capacity checks, and utilization stats.

klingaard · 2024-02-26T17:20:35Z

core/Fetch.cpp

+        {
+            const auto & inst_ptr = inst_generator_->getNextInst(my_clk_);
+            if (SPARTA_EXPECT_TRUE(nullptr != inst_ptr)) {
+                ibuf_.emplace_back(inst_ptr);


Wonder if you can append to the ibuf_ as blocks instead of individual instructions...

arupc-vmicro · 2024-03-04T01:49:03Z

@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?

arupc-vmicro · 2024-03-04T07:33:21Z

core/CPUTopology.cpp

-            "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"


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.

arupc-vmicro · 2024-03-04T07:38:15Z

core/DCache.cpp

-            // 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


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.

arupc-vmicro · 2024-03-04T07:55:50Z

core/Fetch.cpp

        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


I don't understand why we need block_width / 2 as ibuf capacity. Can you please add a comment as an explanation?

arupc-vmicro · 2024-03-04T08:30:31Z

core/Fetch.cpp

+        // 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 */);
+


Can you please clarify why this is needed?

arupc-vmicro · 2024-03-04T08:52:45Z

core/Fetch.cpp

    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.


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?

jhsiaovt · 2024-03-04T21:14:07Z

core/ICache.hpp

+        void getRequestFromFetch_(const MemoryAccessInfoPtr &);
+        void getCreditsFromL2Cache_(const uint32_t &);
+        void getRespFromL2Cache_(const MemoryAccessInfoPtr &);


Why callbacks? Why are in ports below not enough?

jeffnye-gh · 2024-08-30T18:40:05Z

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.

Signed-off-by: danbone <[email protected]> Co-authored-by: Daniel Bone <[email protected]> Co-authored-by: danbone <[email protected]>

arupc · 2024-11-10T10:19:59Z

Since Knute and Jeff has reviewed this, I will unassign myself from this review. Please feel free to mege if all comments from other reviewers have been addressed.

klingaard · 2024-11-10T19:17:41Z

Actually, this was merged in #211. We can safely close this PR as NOT merged.

Daniel Bone added 5 commits February 8, 2024 13:56

Add L1 Instruction Cache

13edff8

Connect up ICache to Fetch

431e74e

Fix rename and issue tests accounting for icache latency

b09fd6c

Fix LSU test accounting for extra ICache latency

4dee506

Fix L2Cache tests. Change L2+MSS to use credit instead of ack

9c77264

klingaard previously approved these changes Feb 12, 2024

View reviewed changes

Daniel Bone and others added 2 commits February 16, 2024 12:51

Add fetch block size, and buffer size parameters to Fetch

7fd5471

Merge branch 'master' into danbone/issue_143_icache

58a9f4c

Signed-off-by: danbone <[email protected]>

danbone marked this pull request as ready for review February 16, 2024 15:58

danbone requested a review from klingaard February 16, 2024 19:04

Merge remote-tracking branch 'upstream/master' into danbone/issue_143…

063a6f3

…_icache

danbone mentioned this pull request Feb 21, 2024

Add decoupled branch predictor #155

Closed

arupc-vmicro reviewed Feb 26, 2024

View reviewed changes

core/DCache.cpp Show resolved Hide resolved

arupc-vmicro self-requested a review February 26, 2024 03:41

Daniel Bone added 2 commits February 26, 2024 14:48

update dcache comments on L2Cache credits

50acbb1

Merge branch 'danbone/issue_143_icache' of https://github.com/danbone…

b020c81

…/riscv-perf-model into danbone/issue_143_icache

klingaard reviewed Feb 26, 2024

View reviewed changes

arupc-vmicro reviewed Mar 4, 2024

View reviewed changes

arupc-vmicro self-requested a review March 4, 2024 08:53

jhsiaovt reviewed Mar 4, 2024

View reviewed changes

dragon540 mentioned this pull request Sep 29, 2024

Resolve conflicts between PR #152 and branch master #211

Merged

klingaard pushed a commit that referenced this pull request Oct 31, 2024

Resolve conflicts between PR #152 and branch master (#211)

8f9e442

Signed-off-by: danbone <[email protected]> Co-authored-by: Daniel Bone <[email protected]> Co-authored-by: danbone <[email protected]>

arupc removed the request for review from arupc-vmicro November 10, 2024 10:20

klingaard closed this Nov 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add L1 Instruction Cache #152

Add L1 Instruction Cache #152

danbone commented Feb 8, 2024 •

edited

Loading

klingaard left a comment •

edited

Loading

danbone commented Feb 16, 2024

klingaard Feb 26, 2024

klingaard Feb 26, 2024

arupc-vmicro commented Mar 4, 2024

arupc-vmicro Mar 4, 2024

arupc-vmicro Mar 4, 2024

arupc-vmicro Mar 4, 2024

arupc-vmicro Mar 4, 2024

arupc-vmicro Mar 4, 2024

jhsiaovt Mar 4, 2024

jeffnye-gh commented Aug 30, 2024

arupc commented Nov 10, 2024

klingaard commented Nov 10, 2024

Add L1 Instruction Cache #152

Add L1 Instruction Cache #152

Conversation

danbone commented Feb 8, 2024 • edited Loading

klingaard left a comment • edited Loading

Choose a reason for hiding this comment

danbone commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arupc-vmicro commented Mar 4, 2024

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

jeffnye-gh commented Aug 30, 2024

arupc commented Nov 10, 2024

klingaard commented Nov 10, 2024

danbone commented Feb 8, 2024 •

edited

Loading

klingaard left a comment •

edited

Loading