Sort RR graph edges with stable counting sort by soheilshahrouz · Pull Request #3800 · verilog-to-routing/vtr-verilog-to-routing

soheilshahrouz · 2026-09-02T22:47:33Z

Sorting RR graph edges by destination node and by switch configurability used std::stable_sort with a comparator, which is slow for the tens of millions of edges in large graphs.

This PR adds vtr_sort.h with a stable counting sort. t_rr_graph_storage::sort_edges is replaced with sort_edges_by_keys, which takes one or more small integer keys and sorts with counting sort passes instead of a comparison sort. Both edge sorts in rr_graph_storage and the RR graph serializer now use it.

The resulting edge order is identical to the old stable sort, so RR graph contents and QoR are unchanged.

…::stable_sort

…for both edge sorts

…ed algorithm include

…g_sort

soheilshahrouz · 2026-09-02T22:49:04Z

Impact on RR graph generation time
stratixiv_arch.timing.xml, --route_chan_width 300

Device width	master (s)	branch (s)	Ratio
100	27.55	23.07	0.837
200	113.20	92.21	0.815
250	173.25	139.70	0.806
300	256.22	204.30	0.797
geomean			0.814

soheilshahrouz · 2026-09-02T22:50:29Z

Impact on reading binary RR graph files
The same architecture and channel width as the previous comment.

Device width	File size	master (s)	branch (s)	Ratio
100	0.85 GB	7.65	7.26	0.949
200	3.4 GB	31.85	29.75	0.934
250	5.3 GB	48.73	45.80	0.940
geomean				0.941

vaughnbetz

Nice results and the code looks good. I feel like I just failed a C++ test, but I don't see ways to make it simpler :). I have a couple of commenting suggestions but that's it. Probably want to make sure this shows up in the web vtrutil documentation too.

AmirhosseinPoolad · 2026-09-03T00:02:06Z

+ * @param num_keys  Exclusive upper bound on the keys. Every key must be smaller than this.
+ */
+template<typename InIt, typename OutIt, typename KeyFn>
+void stable_counting_sort(InIt first, InIt last, OutIt out, size_t num_keys, KeyFn key_of) {


I think explaining the function arguments instead of template params would be more clear and also more consistent. possibly could even have both.

AmirhosseinPoolad · 2026-09-03T00:08:28Z

+
+    for (InIt it = first; it != last; ++it) {
+        size_t key = static_cast<size_t>(key_of(*it));
+        out[offsets[key]++] = *it;


This was hard to understand for me. I think if 'offsets[key] += 1' was in a separate line it would've been much easier to understand.

AmirhosseinPoolad · 2026-09-03T00:09:19Z

+    for (size_t key = 1; key <= num_keys; ++key) {
+        offsets[key] += offsets[key - 1];
+    }


You should add a high level comment explaining the overall algorithm stages (counting, then prefix sum then writing to output)

AmirhosseinPoolad · 2026-09-03T00:18:35Z

+template<typename KeyFn>
+sort_key(size_t, KeyFn) -> sort_key<KeyFn>;


Add a comment explaining this. I think it's here to help the type system deduce the type, but the syntax is pretty uncommon.

AmirhosseinPoolad · 2026-09-03T00:20:29Z

+    stable_counting_sort(first, last, std::begin(out), least_significant.num_keys, least_significant.key_of);
+
+    // Remaining passes ping-pong between out and scratch, ending in out
+    if constexpr (num_keys > 1) {


Super nice use of if constexpr! However I think some comments for what this means and why it's here would be nice here, this feature relatively new to C++ and people might not be familiar with it.

AmirhosseinPoolad · 2026-09-03T00:23:38Z

+ * @param num_keys  Exclusive upper bound on the keys. Every key must be smaller than this.
+ */
+template<typename Container, typename KeyFn>
+void stable_counting_sort(Container& items, Container& scratch, size_t num_keys, KeyFn key_of) {


I wonder if this and the other function shouldn't be in the details namespace. radix sort with one key compiles to just calling counting sort, with the added benefit of not having to manage a scratch memory. There also aren't any users of this function outside this file.

AlexandreSinger

Thank you so much @soheilshahrouz , I really really love this PR a ton.

I had some comments on how this can be made a bit clearer. Other than that, it makes sense to me.

I did have a comment on using uint32_t for offests in this function. This would limit us to RR graphs with 4 billion nodes. I think in the past we always use size_t which can be as many as 2^64; but correct me if I am wrong here.

AlexandreSinger · 2026-09-03T02:50:53Z

+ * such inputs are a poor match for this function.
+ *
+ * @tparam InIt   Forward iterator over the input elements.
+ * @tparam OutIt  Random access iterator to the output range.


I think it would be nice to assert that InIt and OutIt are correct iterator types for this method. If we are going to go full C++, we may as well go all out LOL.

This would look like this within the function:

static_assert(std::forward_iterator<InIt>); static_assert(std:: random_access_iterator <OutIt>);

This has no runtime overhead since these asserts will be checked at compile-time. I think this is possible in our version of C++.

Actually, I just saw the "requires" down below. If you already considered this and it did not work I understand; but I think it would be good to have these requires in here if we can. Better safe.

AlexandreSinger · 2026-09-03T03:02:25Z

+    /// Exclusive upper bound on the keys
+    size_t num_keys;
+    /// Callable mapping an element to its key
+    KeyFn key_of;


Instead of a full template here, I would opt for an std::function with templated arguments. It self-documents what this is expected to be.

Correct me if I am wrong, but I think the type for this should be:

std::function<size_t(KeyType)> key_of;

I think this should be done throughout this file. It adds explicit type-casting and I would find it easier to follow.

AlexandreSinger · 2026-09-03T03:07:43Z

-    template <typename t_comp_func>
-    void sort_edges(t_comp_func comparison_function) {
-
+    template <typename... KeyFns>


I personally would add an example for how to use this when there are multiple sort_keys are passed in. Variadic arguments in C++ can be very confusing.

I would also add an @param here explaining this argument. Its fun, but can be confusing.

AlexandreSinger · 2026-09-03T03:15:29Z

+        VTR_ASSERT(new_index == vec.size());
+
+        vec = std::move(new_vec);
+    };


What is the benefit of making this a lambda? I am not entirely sure, but frankly, I think this lambda may just be compiled down into a templated static function due to the hackary we are doing with remove_refernece. Why not just do that directly?

Something like:

template<typename T> void array_rearange(std::vector<T>& vec, T default_value) { std::vector<T> new_vec(...); ... }

I think doing this will make this much easier to follow.

AlexandreSinger · 2026-09-03T03:23:20Z

+ * counting sort pass per key, least significant key first. The input is read
+ * directly, so a generated sequence such as a vtr::StrongIdRange needs no copy first.
+ *
+ * Example, sorting edges by (source node, destination node):


Could you explain the example slightly more. I know its a bit of a pain, but I think it would be helpful to say something like "such that edges with the lower source ID, then the lowest destination ID, are put first in sorted edges". It just provides a concrete example of how to read this, since many sorting functions in std allow you to change if you want to go from lowest first or highest first.

On that note: it is currently not clear whether this is lowest first or highest first. You may want to specify.

AlexandreSinger · 2026-09-03T03:31:45Z

+
+    size_t num_elements = 0;
+    for (InIt it = first; it != last; ++it) {
+        size_t key = static_cast<size_t>(key_of(*it));


This is a bit tricky... You are assuming that the key_of function's result is always castable to a size_t. You may want to document that somewhere. That is actually something that people may find confusing since they may want to sort on a floating-point key for example.

Obviously a floating-point key does not work for counting-sort; but the user may not be aware of that. I think there is a static assert you can use to ensure that the result of key_of is at least an integer type.

AlexandreSinger · 2026-09-03T03:34:35Z

+    size_t num_elements = 0;
+    for (InIt it = first; it != last; ++it) {
+        size_t key = static_cast<size_t>(key_of(*it));
+        VTR_ASSERT_MSG(key < num_keys, "Sort key must be smaller than num_keys");


Up to you: This is in the hot loop. I recommend graduating it to VTR_ASSERT_DEBUG.

The other asserts are fine in my opinion since they happen once per sort call, but I will leave this up to you. I understand if you want to keep this since this would be explosive if incorrect.

AlexandreSinger · 2026-09-03T03:36:14Z

+        num_elements++;
+    }
+    VTR_ASSERT_MSG(num_elements <= std::numeric_limits<uint32_t>::max(),
+                   "Number of sorted elements must fit in the 32 bit offsets");


Why 32-bit offsets? Does this mean that the vectors being sorted can only be at most length 2^32? I would document this limitation if that is the case.

I think size_t can be as high as 64 bits on a 64 bit operating system. And I am pretty sure we can have RR graphs bigger than 4 billion nodes; but maybe I am incorrect here.

soheilshahrouz added 13 commits August 24, 2026 00:04

Add vtr_sort.h with stable counting sort and LSD radix sort

28c5038

Assert that items and scratch are distinct in stable_counting_sort

285e700

Size the internal scratch buffer instead of copying items into it

020f1d5

Count counting sort offsets in uint32_t instead of size_t

c501db5

Sort RR edges by destination node with a counting sort instead of std…

503da6f

…::stable_sort

Sort RR edges by any list of keys with sort_edges_by_keys and use it …

ec9bc9d

…for both edge sorts

Remove the unused comparator based sort_edges from t_rr_graph_storage

52392c1

Include vtr_sort.h directly in rr_graph_storage.cpp and drop the unus…

13a58d6

…ed algorithm include

Check edge sink nodes and switch ids before partitioning the edges

2990d8a

Merge remote-tracking branch 'origin/master' into rr_gen_edge_countin…

1ee91a1

…g_sort

Remove unused container overloads of stable_radix_sort

0834004

Check sort keys against num_keys in release builds

626b7c0

Merge remote-tracking branch 'origin/master' into rr_gen_edge_countin…

6b7abf3

…g_sort

github-actions Bot added lang-cpp C/C++ code libvtrutil labels Sep 2, 2026

soheilshahrouz requested a review from vaughnbetz September 2, 2026 22:50

vaughnbetz approved these changes Sep 2, 2026

View reviewed changes

Comment thread libs/librrgraph/src/base/rr_graph_storage.cpp Outdated

Comment thread libs/libvtrutil/src/vtr_sort.h

soheilshahrouz added 2 commits September 2, 2026 19:52

Document the complexity and memory cost of stable_counting_sort

47aaae1

Fix a typo in an rr_graph_storage comment

727d10b

AmirhosseinPoolad reviewed Sep 3, 2026

View reviewed changes

AlexandreSinger reviewed Sep 3, 2026

View reviewed changes

		template<typename KeyFn>
		sort_key(size_t, KeyFn) -> sort_key<KeyFn>;

Uh oh!

Conversation

soheilshahrouz commented Sep 2, 2026

Uh oh!

soheilshahrouz commented Sep 2, 2026

Uh oh!

soheilshahrouz commented Sep 2, 2026

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexandreSinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants