DynamoRIO / DynamoRIO/dynamorio
New drcachesim replacement policy code passes wrong values for set count
- Dominant language
- C
- Stars
- 3.2k
- Forks
- 629
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 31
Description
PR #7314 refactoring the cache replacement code to add a new interface cache_replacement_policy_t.
The cache_replacement_policy_t class takes in the number of sets and the associativity.
But the number of sets passed in seems incorrect in multiple places:
When cache_simulator_t creates caches it passes the total line count instead of the lines per way:
```
create_cache_replacement_policy(
knobs_.replace_policy, (int)knobs_.LL_size / (int)knobs_.line_size,
```
Further confusing things is that caching_device_t takes in the total line count==block count, but the replacement policy does seem to want the count per way, as policies like LRU expect associativity per set:
```
// Initialize the LRU list for each set.
lru_counters_.reserve(num_sets);
for (int i = 0; i < num_sets; ++i) {
lru_counters_.emplace_back(associativity, 1);
}
```
The TLB simulator seems correct since there is no "line size" and the total size is in entry counts:
```
auto replace_policy = create_cache_replacement_policy(
knobs_.TLB_replace_policy, knobs_.TLB_L1I_entries / knobs_.TLB_L1I_assoc,
```
Some new tests also seem incorrect, in a different way from the cache_simulator_t:
```
void
unit_test_cache_lru_four_way()
{
caching_device_policy_test_t cache_lru_test(/*associativity=*/4,
/*line_size=*/32);
cache_lru_test.initialize_cache(std::unique_ptr(new policy_lru_t(
/*num_blocks=*/256 / 4, /*associativity=*/4)),
256);
```
That test says cache size is 256 bytes, line size is 32 bytes, assoc is 4. That should mean 2 lines == sets for each of the 4 ways. But it passes num_sets to policy_lru_t (calling it num_blocks) as 256/4 == 64.
Most likely these tests do not depend on the set count and are only really checking replacement within a particular line, so this was not detected by the tests.
Contributor guide
Assessment
This issue has not been assessed yet.