Bug fixes: serial-lookup refill collision, SCM tie-off multi-driver, and fetch-merge address mismatch#39
Open
liyinrong wants to merge 3 commits into
Open
Bug fixes: serial-lookup refill collision, SCM tie-off multi-driver, and fetch-merge address mismatch#39liyinrong wants to merge 3 commits into
liyinrong wants to merge 3 commits into
Conversation
When a lookup request reaches the data stage in the same cycle a refill writes its line, the single-port data SRAM prioritises the write and the request's read is preempted, so `data_rdata` holds stale bytes from an earlier read. The output select `refill_hit_q && !data_req_q.hit` trusted that stale read whenever the tag also hit, so the request was served another line's data. Under heavy, rapidly-interleaved fetch traffic (many cores thrashing the same hot lines through a shared read-only cache) this returned wrong instructions and could hang the system. The tag-hit-plus-coincident-refill case looks contradictory (why refill a line that is already valid?) but is reachable: the miss handler decides hit/miss from the lookup result, which is two cycles latent. A request that missed a line before it was cached can still be in flight to the handler when the earlier refill completes and frees its pending/MSHR entry, so the coalescing check misses it and a second, redundant refill is issued for the now-valid line. That redundant write is what collides with a later request that already tag-hits the line. - Serve the in-flight refill data on any refill_hit_q (drop the !data_req_q.hit qualifier on proper_rdata/out_way_o/out_error_o). - Qualify the refill-payload capture (refill_wdata_q/write_way_q/ write_error_q) with the tag handshake so a later request's refill cannot overwrite it while the owning request is stalled in the data stage. The data output is already shielded by the data_rsp_q fall-through buffer; this protects the un-buffered way/error outputs.
In the MERGE_FETCHES path, prefetch_req_ready[i] also OR'd in the arbiter grant prefetch_req_ready_tmp[i], so a fetch port was acknowledged even when a different-address request won the shared lookup mux — its prefetch was consumed without ever being looked up. Gate the ready strictly on an address match with the accepted lookup request, so a port is acknowledged only when its own address is the one being served.
There was a problem hiding this comment.
Pull request overview
Fixes three correctness issues in the Snitch I-cache datapath/control that could cause dropped fetches, multiple drivers, or stale data being served during coincident refills.
Changes:
- Correct fetch-merge ready signaling so a fetch port is only acknowledged when its own address is the accepted lookup address (
MERGE_FETCHESpath). - Fix multiple-driver issue by tying off
sram_cfg_out_tag_oexactly once in the SCM tag implementation branch. - Fix serial-lookup refill collision by always serving refill payload on
refill_hit_q(even on tag hits) and by latching refill payload/metadata only on a tag-stage handshake to prevent overwrite during stalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/snitch_icache.sv | Fixes MERGE_FETCHES ready gating to prevent acknowledging mismatched-address fetches. |
| src/snitch_icache_lookup_serial.sv | Fixes SCM tag config multi-driver and corrects serial lookup behavior on coincident refill writes (data/way/error selection + safe payload capture). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Serial lookup serves stale data on a coincident refill write
When a lookup request reaches the data stage in the same cycle a refill writes its line, the single-port data SRAM prioritises the write and the request's read is preempted, so
data_rdataholds stale bytes from an earlier read. The output selectrefill_hit_q && !data_req_q.hittrusted that stale read whenever the tag also hit, so the request was served another line's data.The tag-hit-plus-coincident-refill case is reachable via the two-cycle lookup latency: a request that missed a line before it was cached can still be in flight to the handler when the earlier refill completes and frees its pending/MSHR entry, so a second, redundant refill is issued for the valid line, colliding with a later request that already tag-hits it.
Fix:
refill_hit_q(drop the!data_req_q.hitqualifier onproper_rdata/out_way_o/out_error_o).refill_wdata_q/write_way_q/write_error_q) with the tag handshake (refill_hit_d && tag_valid && tag_ready) so a later request's refill cannot overwrite it while the owning request is stalled in the data stage.out_data_ois already shielded by thedata_rsp_qfall-through buffer; this protects the un-buffered way/error outputs.2. Serial SCM tag config tie-off is a multiple driver
In the
gen_scm(latch-based tag SCM) branch,assign sram_cfg_out_tag_o = '0;sat inside theg_waysgenvar loop, so the output was drivenWAY_COUNTtimes.Fix: move the tie-off out of the loop so it is driven exactly once.
3. Unexpected fetch merge under address mismatch
In the
MERGE_FETCHESpath,prefetch_req_ready[i]OR'd in the arbiter grantprefetch_req_ready_tmp[i], so a fetch port was acknowledged even when a different-address request won the shared lookup mux — that port's prefetchwas consumed without ever being looked up.
Fix: gate the ready strictly on an address match with the accepted lookup request, so a port is acknowledged only when its own address is the one being served.