Skip to content

Add a NestedNetlistRouter to enable integration with fine grained parallel router #2924

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

Merged
merged 1 commit into from
Mar 31, 2025

Conversation

duck2
Copy link
Contributor

@duck2 duck2 commented Mar 6, 2025

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.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code libvtrutil labels Mar 6, 2025
@duck2 duck2 changed the title Add vtr::thread_pool and an option to use it with parallel routers [WIP] Add vtr::thread_pool and an option to use it with parallel routers Mar 6, 2025
@duck2 duck2 changed the title [WIP] Add vtr::thread_pool and an option to use it with parallel routers Add a MixedNetlistRouter to enable integration with fine grained parallel router Mar 18, 2025
Copy link
Contributor

@vaughnbetz vaughnbetz left a 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.

std::thread thread;
std::queue<std::function<void()>> task_queue;
std::mutex queue_mutex;
std::condition_variable cv;
Copy link
Contributor

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;
Copy link
Contributor

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) {
Copy link
Contributor

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.

@duck2 duck2 changed the title Add a MixedNetlistRouter to enable integration with fine grained parallel router Add a NestedNetlistRouter to enable integration with fine grained parallel router Mar 20, 2025
@duck2 duck2 force-pushed the custom-thread-pool branch from dab0030 to c6a5902 Compare March 20, 2025 04:48
@duck2
Copy link
Contributor Author

duck2 commented Mar 20, 2025

QoR data: custom_thread_pool_5.xlsx So NestedNetlistRouter gets the same results with ParallelNetlistRouter but it takes ~4.4% more time.

@vaughnb-cerebras
Copy link

Looks good. We can merge once the commenting and dead code feedback is addressed / resolved.

@duck2 duck2 force-pushed the custom-thread-pool branch from 01dbbfc to ac3fe5b Compare March 27, 2025 16:50
@duck2 duck2 force-pushed the custom-thread-pool branch from ac3fe5b to 0c7800b Compare March 31, 2025 16:09
Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

Thanks for the additional commenting. I think you should add per function doxygen comments to the thread pool though (there are still some member functions with no comment).

@vaughnbetz vaughnbetz merged commit 2311863 into master Mar 31, 2025
36 checks passed
@vaughnbetz vaughnbetz deleted the custom-thread-pool branch March 31, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code libvtrutil VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants