Skip to content

Exclude built-in nodes from forward_class and align traversal tests #194

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.

Sign up for GitHub

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
renau merged 1 commit into from
Mar 5, 2026
Merged

Exclude built-in nodes from forward_class and align traversal tests #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions hhds/graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,8 +770,9 @@ void Graph::rebuild_fast_hier_cache() const {
}

void Graph::rebuild_forward_class_cache() const {
constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST
forward_class_cache_.clear();
if (node_table.size() <= 1) {
if (node_table.size() <= first_user_node_idx) {
forward_class_cache_valid_ = true;
Comment on lines +773 to 776
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

first_user_node_idx is a magic number that is now duplicated here and again in Graph::forward_class(). To avoid the two code paths drifting (and causing cache-size mismatches / repeated rebuilds if one is updated without the other), consider defining a single shared constant (e.g., a static constexpr on Graph or a file-scope constant) and using it in both places.

Copilot uses AI. Check for mistakes.
return;
}
Expand All @@ -791,7 +792,7 @@ void Graph::rebuild_forward_class_cache() const {

sink_nid &= ~static_cast<Nid>(3);
const size_t sink_idx = static_cast<size_t>(sink_nid >> 2);
if (sink_idx == 0 || sink_idx >= node_count) {
if (sink_idx < first_user_node_idx || sink_idx >= node_count) {
return;
}

Expand All @@ -800,7 +801,7 @@ void Graph::rebuild_forward_class_cache() const {
}
};

for (size_t driver_idx = 1; driver_idx < node_count; ++driver_idx) {
for (size_t driver_idx = first_user_node_idx; driver_idx < node_count; ++driver_idx) {
const Nid driver_nid = static_cast<Nid>(driver_idx) << 2;

auto node_edges = node_table[driver_idx].get_edges(driver_nid);
Expand All @@ -825,14 +826,14 @@ void Graph::rebuild_forward_class_cache() const {
}

std::priority_queue<size_t, std::vector<size_t>, std::greater<size_t>> ready;
for (size_t idx = 1; idx < node_count; ++idx) {
for (size_t idx = first_user_node_idx; idx < node_count; ++idx) {
if (indegree[idx] == 0) {
ready.push(idx);
}
}

std::vector<bool> emitted(node_count, false);
forward_class_cache_.reserve(node_count - 1);
forward_class_cache_.reserve(node_count - first_user_node_idx);
while (!ready.empty()) {
const size_t node_idx = ready.top();
ready.pop();
Expand All @@ -855,7 +856,7 @@ void Graph::rebuild_forward_class_cache() const {
}

// Preserve determinism under cycles by appending unresolved nodes by ID.
for (size_t idx = 1; idx < node_count; ++idx) {
for (size_t idx = first_user_node_idx; idx < node_count; ++idx) {
if (!emitted[idx]) {
forward_class_cache_.emplace_back(const_cast<Graph*>(this), static_cast<Nid>(idx) << 2);
}
Expand Down Expand Up @@ -1099,7 +1100,8 @@ auto Graph::fast_class() const -> std::span<const Node_class> {
}

auto Graph::forward_class() const -> std::span<const Node_class> {
const size_t expected_size = node_table.size() > 0 ? node_table.size() - 1 : 0;
constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST
const size_t expected_size = node_table.size() > first_user_node_idx ? node_table.size() - first_user_node_idx : 0;
if (!forward_class_cache_valid_ || forward_class_cache_.size() != expected_size) {
Comment on lines +1103 to 1105
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

first_user_node_idx is re-declared here even though rebuild_forward_class_cache() also declares it. Centralizing this constant (ideally near the built-in node definitions) would reduce the risk of future inconsistencies in expected cache size vs. cache contents.

Copilot uses AI. Check for mistakes.
rebuild_forward_class_cache();
}
Expand Down
2 changes: 1 addition & 1 deletion hhds/tests/iterators_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ TEST(LazyTraversal, ForwardClassTopologicalOrder) {
auto order2 = g.forward_class();
EXPECT_EQ(order.data(), order2.data());

ASSERT_EQ(order.size(), 7);
ASSERT_EQ(order.size(), 4);
absl::flat_hash_map<hhds::Nid, size_t> pos;
for (size_t i = 0; i < order.size(); ++i) {
pos[order[i].get_raw_nid() & ~static_cast<hhds::Nid>(3)] = i;
Expand Down
Loading