-
Notifications
You must be signed in to change notification settings - Fork 406
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
Add a NestedNetlistRouter to enable integration with fine grained parallel router #2924
base: master
Are you sure you want to change the base?
Conversation
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.
Some commenting and renaming suggestions.
Also add QoR data to the PR.
namespace vtr { | ||
|
||
class thread_pool { | ||
private: |
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 every member variable with doxygen and the overall class. I'd give an example of how to use the class in the overall comment.
This should also go into the vtr utility documentation as a new tab eventually.
std::condition_variable cv; | ||
bool stop = false; | ||
size_t thread_id; // For debugging | ||
size_t tasks_completed{0}; // Track number of completed tasks |
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.
For logging, or something else?
std::thread thread; | ||
std::queue<std::function<void()>> task_queue; | ||
std::mutex queue_mutex; | ||
std::condition_variable cv; |
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.
Shouldn't the name give an idea of what the condition variable controls/is used for, rather than just cv?
std::atomic<size_t> total_tasks_queued{0}; | ||
vtr::Timer pool_timer; // Track pool lifetime | ||
std::atomic<size_t> active_tasks{0}; | ||
std::mutex completion_mutex; |
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 these. (all the variables).
From our discussion: completion_mutex is to wait for all the tasks in the queue to finish.
std::condition_variable completion_cv; | ||
|
||
public: | ||
thread_pool(size_t thread_count) { |
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.
Between the class overview and here, you need to comment how this thread pool works.
vpr/src/route/MixedNetlistRouter.tpp
Outdated
@@ -0,0 +1,141 @@ | |||
#pragma once | |||
|
|||
/** @file Impls for ParallelNetlistRouter */ |
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.
NestedPNetlistRouter
vpr/src/route/MixedNetlistRouter.h
Outdated
RouteIterResults route_netlist(int itry, float pres_fac, float worst_neg_slack); | ||
/** Inform the PartitionTree of the nets with updated bounding boxes */ | ||
void handle_bb_updated_nets(const std::vector<ParentNetId>& nets); | ||
void set_rcv_enabled(bool x); |
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.
Put a comment on set_rcv_enabled that it sets this control variable for each thread (in its local storage).
vpr/src/route/MixedNetlistRouter.h
Outdated
void handle_bb_updated_nets(const std::vector<ParentNetId>& nets); | ||
void set_rcv_enabled(bool x); | ||
void set_timing_info(std::shared_ptr<SetupHoldTimingInfo> timing_info); | ||
|
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.
Same for set_timing_info --> explain it does this for all threads.
vpr/src/route/netlist_routers.h
Outdated
@@ -71,6 +71,7 @@ class NetlistRouter { | |||
|
|||
/* Include the derived classes here to get the HeapType-templated impls */ | |||
#include "SerialNetlistRouter.h" | |||
#include "MixedNetlistRouter.h" |
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.
NestedPNetlistRouter
vpr/src/route/netlist_routers.h
Outdated
@@ -104,6 +105,20 @@ inline std::unique_ptr<NetlistRouter> make_netlist_router_with_heap( | |||
routing_predictor, | |||
choking_spots, | |||
is_flat); | |||
} else if (router_opts.router_algorithm == e_router_algorithm::MIXED) { |
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.
NESTED_PARALLEL
dab0030
to
c6a5902
Compare
QoR data: custom_thread_pool_5.xlsx So |
Looks good. We can merge once the commenting and dead code feedback is addressed / resolved. |
TBB's thread pool doesn't let us block in tasks, so we have to implement a custom thread pool if we want to integrate a parallel netlist router with a parallel connection router.