Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#7291: support large pages in TLB #7292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+42 −28
Open

i#7291: support large pages in TLB #7292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c3a719
i7291: support large pages in TLB
oralmer Feb 20, 2025
a83b0bc
fix format in utils.h
oralmer Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions clients/drcachesim/common/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@ namespace drmemtrace {
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i7291: support large pages in TLB

style nit: s/i7291/i#7291/


static inline int
compute_log2(int value)
compute_log2(long int value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long int is 32 bits in Visual Studio so it is not the right cross-platform type: use int64_t.

Ditto throughout the PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add an issue replace uses of types with compiler defined sizes in general? I usually prefer to use only stdint types and size_t, but I tried to stay consistent with the code here.
I will replace this of course.

{
int i;
for (i = 0; i < 31; i++) {
if (value == 1 << i)
for (i = 0; i < 63; i++) {
if (value == 1l << i)
return i;
}
// returns -1 if value is not a power of 2.
Expand Down
8 changes: 4 additions & 4 deletions clients/drcachesim/simulator/cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ namespace drmemtrace {
class snoop_filter_t;

bool
cache_t::init(int associativity, int line_size, int total_size, caching_device_t *parent,
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't total_size, plus the local num_lines below, also need to be 64-bit?

Ditto below.

caching_device_t *parent, caching_device_stats_t *stats,
prefetcher_t *prefetcher, cache_inclusion_policy_t inclusion_policy,
bool coherent_cache, int id, snoop_filter_t *snoop_filter,
const std::vector<caching_device_t *> &children)
{
// Check line_size to avoid divide-by-0.
Expand Down
2 changes: 1 addition & 1 deletion clients/drcachesim/simulator/cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class cache_t : public caching_device_t {
// The id is an index into the snoop filter's array of caches for coherent caches.
// If this is a coherent cache, id should be in the range [0,num_snooped_caches).
bool
init(int associativity, int line_size, int total_size, caching_device_t *parent,
init(int associativity, long int line_size, int total_size, caching_device_t *parent,
caching_device_stats_t *stats, prefetcher_t *prefetcher = nullptr,
cache_inclusion_policy_t inclusion_policy =
cache_inclusion_policy_t::NON_INC_NON_EXC,
Expand Down
2 changes: 1 addition & 1 deletion clients/drcachesim/simulator/cache_fifo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace drmemtrace {
// be cleared. The counter of the next block will be set to 1.

bool
cache_fifo_t::init(int associativity, int block_size, int total_size,
cache_fifo_t::init(int associativity, long int block_size, int total_size,
caching_device_t *parent, caching_device_stats_t *stats,
prefetcher_t *prefetcher, cache_inclusion_policy_t inclusion_policy,
bool coherent_cache, int id, snoop_filter_t *snoop_filter,
Expand Down
5 changes: 2 additions & 3 deletions clients/drcachesim/simulator/cache_fifo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,11 @@ class cache_fifo_t : public cache_t {
{
}
bool
init(int associativity, int line_size, int total_size, caching_device_t *parent,
init(int associativity, long int block_size, int total_size, caching_device_t *parent,
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a clang-format change across clang-format versions or something?

const std::vector<caching_device_t *> &children = {}) override;

std::string
Expand Down
2 changes: 1 addition & 1 deletion clients/drcachesim/simulator/cache_lru.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ namespace drmemtrace {
// highest counter value will be picked for replacement in replace_which_way.

bool
cache_lru_t::init(int associativity, int block_size, int total_size,
cache_lru_t::init(int associativity, long int block_size, int total_size,
caching_device_t *parent, caching_device_stats_t *stats,
prefetcher_t *prefetcher, cache_inclusion_policy_t inclusion_policy,
bool coherent_cache, int id, snoop_filter_t *snoop_filter,
Expand Down
5 changes: 2 additions & 3 deletions clients/drcachesim/simulator/cache_lru.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,11 @@ class cache_lru_t : public cache_t {
{
}
bool
init(int associativity, int line_size, int total_size, caching_device_t *parent,
init(int associativity, long int block_size, int total_size, caching_device_t *parent,
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,
const std::vector<caching_device_t *> &children = {}) override;
std::string
get_replace_policy() const override
Expand Down
4 changes: 2 additions & 2 deletions clients/drcachesim/simulator/caching_device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
#include <utility>
#include <vector>

#include "memref.h"
#include "caching_device_block.h"
#include "caching_device_stats.h"
#include "memref.h"
#include "prefetcher.h"
#include "snoop_filter.h"
#include "trace_entry.h"
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we widen num_blocks while at it or we are certain 32-bit is plenty?

caching_device_t *parent, caching_device_stats_t *stats,
prefetcher_t *prefetcher,
cache_inclusion_policy_t inclusion_policy, bool coherent_cache,
Expand Down
14 changes: 9 additions & 5 deletions clients/drcachesim/simulator/caching_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,11 @@ class prefetcher_t;

// NON_INC_NON_EXC = Non-Inclusive Non-Exclusive, aka NINE.
enum class cache_inclusion_policy_t { NON_INC_NON_EXC, INCLUSIVE, EXCLUSIVE };

class caching_device_t {
public:
explicit caching_device_t(const std::string &name = "caching_device");
virtual bool
init(int associativity, int block_size, int num_blocks, caching_device_t *parent,
init(int associativity, long int block_size, int num_blocks, caching_device_t *parent,
caching_device_stats_t *stats, prefetcher_t *prefetcher = nullptr,
cache_inclusion_policy_t inclusion_policy =
cache_inclusion_policy_t::NON_INC_NON_EXC,
Expand Down Expand Up @@ -110,6 +109,11 @@ class caching_device_t {
{
return parent_;
}
void
set_parent(caching_device_t *parent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to this PR.

{
parent_ = parent;
}
inline double
get_loaded_fraction() const
{
Expand Down Expand Up @@ -143,7 +147,7 @@ class caching_device_t {
{
return associativity_;
}
virtual int
virtual long int
get_block_size() const
{
return block_size_;
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

int num_blocks_; // Total number of lines in cache = size / block_size.
bool coherent_cache_;
// This is an index into snoop filter's array of caches.
int id_;
Expand Down
22 changes: 17 additions & 5 deletions clients/drcachesim/simulator/tlb_simulator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,27 @@ tlb_simulator_t::tlb_simulator_t(const tlb_simulator_knobs_t &knobs)

if (!itlbs_[i]->init(knobs_.TLB_L1I_assoc, (int)knobs_.page_size,
knobs_.TLB_L1I_entries, lltlbs_[i],
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_ =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

"Usage error: failed to initialize itlbs_. Ensure entry number, "
"page size and associativity are powers of 2.";
success_ = false;
return;
}
if (!dtlbs_[i]->init(knobs_.TLB_L1D_assoc, (int)knobs_.page_size,
knobs_.TLB_L1D_entries, lltlbs_[i],
new tlb_stats_t((int)knobs_.page_size)) ||
!lltlbs_[i]->init(knobs_.TLB_L2_assoc, (int)knobs_.page_size,
new tlb_stats_t((int)knobs_.page_size))) {
error_string_ =
"Usage error: failed to initialize dtlbs_. Ensure entry number, "
"page size and associativity are powers of 2.";
success_ = false;
return;
}
if (!lltlbs_[i]->init(knobs_.TLB_L2_assoc, (int)knobs_.page_size,
knobs_.TLB_L2_entries, NULL,
new tlb_stats_t((int)knobs_.page_size))) {
error_string_ =
"Usage error: failed to initialize TLbs_. Ensure entry number, "
"Usage error: failed to initialize lltlbs_. Ensure entry number, "
"page size and associativity are powers of 2.";
success_ = false;
return;
Expand Down
Loading