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.

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

PcCountPair tracker and LoopPoint #255

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

PcCountPair tracker and LoopPoint #255

wants to merge 20 commits into from

Conversation

studyztp
Copy link
Contributor

This branch has a tracker that is focused on tracking the times a set of specific Program Counter addresses have been executed in the multithread workload simulation, and standard library modules that enable LoopPoint sampling execution.
It also has a new parameter called PcCountPair that stores a Program Counter address and a value of count.
Detail explanation and example scripts are included in the documentation.md file.

Change-Id: Ib2e3f376bbbc48fc9a37036fd63e5dd39d72aa67
Change-Id: Id463388a150628c0ee331fb07d5813d99e70f016
Change-Id: I3f43350c7c68ea57cd673a62a6e1c4c0aaa420dd
Change-Id: I06dc3a04795eafeba586a5ba9da21a7f254eafe0
Change-Id: I273d531cc9aee337c0092bcc49cbfea4a129da65
Change-Id: Ic0b22083d8ceb7e70e16a087ac23f1df8be5c4f0
Change-Id: I08e7d14d88c6d4ff8a84158acc15dbb7d5d43e92
Change-Id: I46a0f867dbab82d7c242b9752da637de29cdc14b
Change-Id: I47730b77ef0f9165efb5ff24dab725929e73f7a1
Change-Id: I1f35afc2b672ee5b2db40e392a724be8bb04e364
Change-Id: I1c3c50149b502eec35c8e3e75f16bbd070860faa
Change-Id: Ib9a40029c9e8fce39a0cd496c026b7012994a8cd
…nt instead of the end

Change-Id: If84afeb42222830bb058d1addc92a5265681cd23
Change-Id: I0a9f7ec241b664be49c5cab66bef39ce80079cd1
Copy link
Member

@powerjg powerjg left a comment

Choose a reason for hiding this comment

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

Overall, this is really great! I'm really impressed with how well this has turned out!

I am going to push another commit to fix some of the style issues, but I'm going to let you fix the rest. Make sure you follow the gem5 style guide: https://www.gem5.org/documentation/general_docs/development/coding_style/

More generally, just make sure your style looks like everything else.

@@ -138,6 +138,66 @@ class Cycles
friend std::ostream& operator<<(std::ostream &out, const Cycles & cycles);
};

class PcCountPair
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in its own file. I would make a new file src/cpu/probes/pc_count_pair.hh

size_t operator()(const PcCountPair& item) const
{
size_t xHash = std::hash<int>()(item.pc);
size_t yHash = std::hash<int>()(item.count) << 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why left shift 1?



class PcCountTrackerManager(SimObject):

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring about this. You can probably copy-paste from the documentation.

PyBindMethod("get_current_pc_count_pair"),
]

targets = VectorParam.PcCountPair("the target PC Count pairs")
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring after this class explaining it? You can probably copy-paste from the documentation.md file.

cxx_header = "cpu/probes/pc_count_tracker_manager.hh"
cxx_class = "gem5::PcCountTrackerManager"

cxx_exports = [
Copy link
Member

Choose a reason for hiding this comment

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

These methods should probably be camelCase.

for core in processor.get_cores():
core.add_pc_tracker_probe(self._targets, self._manager)

def update_relatives(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I suggest calling this update_relative_counts

self._json_file[rid]["simulation"]["end"]["relative"] = int(temp)

def output_json_file(
self, input_indent: int = 4, filename: str = "LoopPoint.json"
Copy link
Member

Choose a reason for hiding this comment

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

Default should be machine, not human readable. I suggest making the default no indent.

For the filename, this should output a file relative to outdir.

Comment on lines +90 to +91
current_pair = self._manager.get_current_pc_count_pair()
temp_pair = PcCountPair(current_pair.getPC(), current_pair.getCount())
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the type of current_pair a PcCountPair? I don't understand why the temp_pair is needed.

Comment on lines +127 to +128
current_pair = self._manager.get_current_pc_count_pair()
temp_pair = PcCountPair(current_pair.getPC(), current_pair.getCount())
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, why is temp_pair needed?

# This section is hard-coded to parse the data in the csv file.
# The csv file is assumed to have a constant format.
with open(looppoint_file_path, newline="") as csvfile:
spamreader = csv.reader(csvfile, delimiter=" ", quotechar="|")
Copy link
Member

Choose a reason for hiding this comment

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

needs a better name than spamreader. f or reader or data would all be better

kaustav-goswami pushed a commit that referenced this pull request Oct 31, 2023
Currently the MOESI_AMD_Base-directory transition for system level
atomics sends the response message before the atomic is performed. This
was likely done because atomics are supposed to return the value of the
data *before* the atomic is performed and by simply ordering the actions
this way that was taken care of.

With the new atomic log feature, the atomic values are pulled from the
log by the coalescer on the return path. Therefore, these actions can be
reordered. In fact, it is now necessary that the atomics be performed
before sending the response so that the log is populated and copied by
the response action. This should fix #253 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants