i7291: support large pages in TLB #7292
Conversation
Changed `block_size` to `long int` in order to allow larger pages in TLB. Also added two minor fixes that came up while working on the feature: - Argument name mismatches in `cache_lru_t` and `cache_fifo_t` between the header and body. - Clearer error messages in `tlb_simulator_t`, so you can know exactly what failed to build. Fixes #7291
| @@ -147,11 +147,11 @@ namespace drmemtrace { | |||
| #endif | |||
There was a problem hiding this comment.
i7291: support large pages in TLB
style nit: s/i7291/i#7291/
| @@ -147,11 +147,11 @@ namespace drmemtrace { | |||
| #endif | |||
|
|
|||
| static inline int | |||
| compute_log2(int value) | |||
| compute_log2(long int value) | |||
There was a problem hiding this comment.
long int is 32 bits in Visual Studio so it is not the right cross-platform type: use int64_t.
Ditto throughout the PR.
| caching_device_stats_t *stats, prefetcher_t *prefetcher, | ||
| cache_inclusion_policy_t inclusion_policy, bool coherent_cache, int id, | ||
| snoop_filter_t *snoop_filter, | ||
| cache_t::init(int associativity, long int line_size, int total_size, |
There was a problem hiding this comment.
Wouldn't total_size, plus the local num_lines below, also need to be 64-bit?
Ditto below.
| caching_device_stats_t *stats, prefetcher_t *prefetcher = nullptr, | ||
| cache_inclusion_policy_t inclusion_policy = | ||
| cache_inclusion_policy_t::NON_INC_NON_EXC, | ||
| bool coherent_cache = false, int id_ = -1, | ||
| snoop_filter_t *snoop_filter_ = nullptr, | ||
| bool coherent_cache = false, int id = -1, snoop_filter_t *snoop_filter = nullptr, |
There was a problem hiding this comment.
Is this a clang-format change across clang-format versions or something?
| @@ -74,7 +74,7 @@ caching_device_t::~caching_device_t() | |||
| } | |||
|
|
|||
| bool | |||
| caching_device_t::init(int associativity, int block_size, int num_blocks, | |||
| caching_device_t::init(int associativity, long int block_size, int num_blocks, | |||
There was a problem hiding this comment.
Should we widen num_blocks while at it or we are certain 32-bit is plenty?
| @@ -110,6 +109,11 @@ class caching_device_t { | |||
| { | |||
| return parent_; | |||
| } | |||
| void | |||
| set_parent(caching_device_t *parent) | |||
There was a problem hiding this comment.
This seems unrelated to this PR.
| @@ -245,8 +249,8 @@ class caching_device_t { | |||
| init_blocks() = 0; | |||
|
|
|||
| int associativity_; | |||
| int block_size_; // Also known as line length. | |||
| int num_blocks_; // Total number of lines in cache = size / block_size. | |||
| long int block_size_; // Also known as line length. | |||
There was a problem hiding this comment.
This may need a mention in api/docs/release.dox (line 127) as it could break or require changes in third-party code deriving from this: we don't consider non-interface changes as breaking enough to go bump the major version so I would add a new list in between the two existing lists of "minor compatibility changes" (as was done for prior releases below in that file: e.g. line 574).
| new tlb_stats_t((int)knobs_.page_size)) || | ||
| !dtlbs_[i]->init(knobs_.TLB_L1D_assoc, (int)knobs_.page_size, | ||
| new tlb_stats_t((int)knobs_.page_size))) { | ||
| error_string_ = |
There was a problem hiding this comment.
(Since the field change is a minor compatibility break it might be nicer to isolate it and move these unrelated changes to a separate PR; but not going to fight for it. This review system doesn't have a "non-strongly-requested" option so using parentheses; will leave up to you.)
|
Note that this Alpine job broke separately and is now fixed in PR #7289. |
Changed
block_sizetolong intin order to allow larger pages in TLB. Also added two minor fixes that came up while working on the feature:cache_lru_tandcache_fifo_tbetween the header and body.tlb_simulator_t, so you can know exactly what failed to build.Fixes #7291