openrisc / mor1kx

mor1kx - an OpenRISC 1000 processor IP core
Other
490 stars 146 forks source link

Erroneous ICache hits impact performance counters #123

Open Harshitha172000 opened 3 years ago

Harshitha172000 commented 3 years ago

Icache updates the LRU access variable only if cache hits occur in the read state. There may be cache hits that occur in the state refill or invalidate. CPU gets back its requested data for these hits but the access information for such hits fails to update the LRU algorithm.

Case 1: Cache hit in refill state

image

Case 2: Cache hit in invalidate state

image

stffrdhrn commented 3 years ago

Can you show the LRU updates happening in case 1 and not happening in case 2 in the traces? How can we reproduce this?

Harshitha172000 commented 3 years ago

LRU is not getting correct access information in both cases. Even though there are way_hits as 10 (case 1) and 01 (case 2), this is not updated in the access variable which remains 00. This access variable is input for the lru_cache module.

Checking the following assertions would result in these errors. https://github.com/Harshitha172000/mor1kx/blob/formal/rtl/verilog/mor1kx_icache.v#L565#L584

stffrdhrn commented 3 years ago

I am not sure this is an issue or we should be touching this. For some reasons

  1. LRU is used for a heuristic use when refilling the cache to replace the least recently used cache way. If it's wrong it means we may discard new data, which is not going to cause failures, so I think not critical.
  2. During refill LRU should be consistent so that we always replace the same LRU cache way.
  3. During invalidate we invalidate all ways, so LRU data is not very meaningful.
Harshitha172000 commented 3 years ago

This issue may not be critical from the LRU perspective. All cache hits are not acknowledged to the fetch module, only refill hits and hits during the READ state are considered, so no need to update lru. But the cache_hit_o is independent of state and it's used to update the performance counter unit (PCU) in the ctrl module. This 'cache_hit_o' may wrongly update PCU when hits occur in invalidate state.

stffrdhrn commented 3 years ago

OK, that seems like a different issue, do you want to close this issue and create a new one, or update the description of this issue to indicate the impact on performance counters?

stffrdhrn commented 3 years ago

Can you change the description? "Impact on performance counters" doesn't communicate the issue well.

stffrdhrn commented 3 years ago

Impact is not a good word to start a issue description. Maybe swap it around "Erroneous ICache hits impact performance counters"