Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] Router Lookahead Profiler #2683
base: master
Are you sure you want to change the base?
[WIP] Router Lookahead Profiler #2683
Changes from all commits
739029b
aa5570b
c88364a
d406c57
c86ff7c
ddf34eb
0826ad7
e8ea974
0b5a752
cbe1334
80086ff
c747096
e753094
d9f4ee3
dfab0a8
c9801b7
7fe0b55
c8b196e
ed4460d
ab2c731
2db5325
17ab565
595af59
2f10ec5
9bcad70
c05e498
643811b
1c9bb58
deddc8a
971eebc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add doxygen comment for this (can use ///).
If it's an empty string does it not get printed? (If so, say that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a brief comment saying why this extra data is needed for PROFILE_LOOKAHEAD, and that we conditionally compile it because this is a hot and large data structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a doxygen comment for this, detailing what you know about the function and its arguments (and for sure what the route_profiler does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding a comment saying the number of instances of this data structure can be large and it is in hot code, so extra data only needed to profile/verify the lookahead is #ifdef'd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should have a comment giving an overview of how it works, unless that's already covered in the (to be added) lookahead_profiler.cpp file-scope comment that gives the high-level context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't private member variables initialization be done in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that I should initialize them as empty maps in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I had in mind was initializing them with all sinks ids as key and proper values. However, this can't be done when LookaheadProfiler has a global instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think storing multiple std::string values for each sink id is not memory efficient. For
atom_block_models
,cluster_block_types
,and tile_dimensions
, the number of unique values is very low. Even foratom_block_names
where each unique value appears only a few times, you don't need to store multiple copies of each block name.std::string_view, vtr::interned_string can be used to avoid unnecessary string copies.
It is also possible to store atom and cluster block ids and use global netlist objects to retrieve references to block/model names and tile sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to change these variables to store IDs and type pointers. Dereferencing the pointers may be less runtime-efficient, but to use
std::string_view
, I think I'd have to create a separatestd::set
which contains all thestring_view
s encountered so far to see if we need to fetch the info for the current node, which seems less clean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the function-level comment above this to Doxygen and describe the variables only needed for the lookahead profiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment that some info is only passed in for lookahead profiling (if you want to allow lookahead profiling with RCV).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a blank line before the routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you know enough about this routine to write a doxygen comment it would be good to write one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this member functon is always called after
get_expected_delay_and_cong()
?In this case, I think it is better to replace this member function with another one that computes the scaled congestion and delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always except for the lookahead profiler. I moved the code in
get_expected_delay_and_cong()
into a new functionget_expected_delay_and_cong_ignore_criticality()
, and nowget_expected_delay_and_cong()
just calls that new function + the scaling function. So, all the previous calls toget_expected_delay_and_cong()
remain unmodified and the profiler can callget_expected_delay_and_cong_ignore_criticality()
.Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.