Skip to content

BE-494: HashQL: Loop-breaker selection for recursive inlining#8600

Open
indietyp wants to merge 9 commits intobm/be-482-hashql-remove-logical-not-from-unary-operatorsfrom
bm/be-494-hashql-scc-loop-breaker-inlining
Open

BE-494: HashQL: Loop-breaker selection for recursive inlining#8600
indietyp wants to merge 9 commits intobm/be-482-hashql-remove-logical-not-from-unary-operatorsfrom
bm/be-494-hashql-scc-loop-breaker-inlining

Conversation

@indietyp
Copy link
Copy Markdown
Member

🌟 What is the purpose of this PR?

This PR implements a three-color depth-first search algorithm for directed graphs and integrates it with a loop-breaker selection system for the MIR inliner. The three-color DFS enables cycle detection and postorder traversal, while the loop-breaker system allows the inliner to handle mutually recursive functions by strategically selecting which functions to avoid inlining within strongly connected components (SCCs).

🔗 Related links

  • Related to MIR optimization and inlining strategies for recursive function groups

🔍 What does this change?

  • Adds a new three-color depth-first search implementation in libs/@local/hashql/core/src/graph/algorithms/color/mod.rs with support for cycle detection and visitor callbacks
  • Implements a loop-breaker selection algorithm in libs/@local/hashql/mir/src/pass/transform/inline/loop_breaker.rs that uses scoring heuristics to choose which functions in recursive SCCs should not be inlined
  • Extends the inline pass to process SCCs with loop-breaker awareness, allowing inlining of non-breaker functions while avoiding infinite recursion
  • Adds comprehensive test coverage for both the three-color DFS algorithm and loop-breaker selection
  • Updates the call graph analysis to support querying callers of a function
  • Adds iterator support for SCC members with both immutable and mutable access
  • Changes unary NOT operator representation from ! to ~ in MIR output formatting
  • Extends inline configuration with InlineLoopBreakerConfig for tuning breaker selection heuristics

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

🛡 What tests cover this?

  • Comprehensive unit tests for three-color DFS including cycle detection, postorder traversal, edge filtering, and state accumulation
  • Loop-breaker selection tests covering mutual recursion, cost-based selection, multi-cycle SCCs, and ordering verification
  • Integration tests showing the complete inlining behavior with loop breakers
  • Existing MIR transformation tests updated to reflect the new unary operator formatting

❓ How to test this?

  1. Run the test suite to verify all three-color DFS and loop-breaker functionality
  2. Test with mutually recursive MIR functions to confirm proper breaker selection and inlining behavior
  3. Verify that the inliner no longer gets stuck in infinite loops when processing recursive function groups

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hash Ready Ready Preview, Comment May 8, 2026 8:46am
petrinaut Ready Ready Preview May 8, 2026 8:46am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hashdotdesign Ignored Ignored Preview May 8, 2026 8:46am
hashdotdesign-tokens Ignored Ignored Preview May 8, 2026 8:46am

Copy link
Copy Markdown
Member Author

indietyp commented Mar 31, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 90.28213% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.27%. Comparing base (f37018e) to head (3319cae).

Files with missing lines Patch % Lines
...cal/hashql/core/src/graph/algorithms/tarjan/mod.rs 0.00% 22 Missing ⚠️
...shql/mir/src/pass/transform/inline/loop_breaker.rs 90.00% 9 Missing and 4 partials ⚠️
...ocal/hashql/mir/src/pass/transform/inline/tests.rs 94.62% 9 Missing and 4 partials ⚠️
...ocal/hashql/core/src/graph/algorithms/color/mod.rs 93.24% 5 Missing ⚠️
...al/hashql/core/src/graph/algorithms/color/tests.rs 96.21% 4 Missing and 1 partial ⚠️
...@local/hashql/mir/src/pass/transform/inline/mod.rs 89.65% 1 Missing and 2 partials ⚠️
...local/hashql/mir/src/pass/transform/inline/find.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                                     Coverage Diff                                      @@
##           bm/be-482-hashql-remove-logical-not-from-unary-operators    #8600      +/-   ##
============================================================================================
+ Coverage                                                     63.65%   68.27%   +4.62%     
============================================================================================
  Files                                                          1271      987     -284     
  Lines                                                        137660    93731   -43929     
  Branches                                                       5482     4793     -689     
============================================================================================
- Hits                                                          87627    63996   -23631     
+ Misses                                                        49123    29073   -20050     
+ Partials                                                        910      662     -248     
Flag Coverage Δ
apps.hash-ai-worker-ts 1.41% <ø> (ø)
apps.hash-api 0.00% <ø> (ø)
blockprotocol.type-system ?
local.claude-hooks ?
local.hash-backend-utils 0.00% <ø> (ø)
local.hash-graph-sdk 9.63% <ø> (ø)
local.hash-isomorphic-utils 0.00% <ø> (ø)
rust.error-stack ?
rust.harpc-net ?
rust.harpc-tower ?
rust.harpc-types ?
rust.harpc-wire-protocol ?
rust.hash-codec ?
rust.hash-graph-api 2.52% <ø> (ø)
rust.hash-graph-authorization ?
rust.hash-graph-postgres-store ?
rust.hash-graph-store ?
rust.hash-graph-types ?
rust.hash-graph-validation ?
rust.hashql-ast 87.23% <ø> (ø)
rust.hashql-compiletest 28.26% <ø> (ø)
rust.hashql-core 82.25% <85.96%> (+0.03%) ⬆️
rust.hashql-diagnostics ?
rust.hashql-eval 79.71% <ø> (ø)
rust.hashql-hir 89.06% <ø> (ø)
rust.hashql-mir 91.68% <92.68%> (+0.01%) ⬆️
rust.hashql-syntax-jexpr 94.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 31, 2026

Merging this PR will not alter performance

✅ 80 untouched benchmarks


Comparing bm/be-494-hashql-scc-loop-breaker-inlining (3319cae) with bm/be-482-hashql-remove-logical-not-from-unary-operators (f37018e)

Open in CodSpeed

Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/tests.rs Fixed
@indietyp indietyp force-pushed the bm/be-482-hashql-remove-logical-not-from-unary-operators branch from 51195eb to d6dbdd5 Compare March 31, 2026 20:56
@indietyp indietyp force-pushed the bm/be-494-hashql-scc-loop-breaker-inlining branch from f66d5ce to b4e6aaf Compare March 31, 2026 20:56
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 31, 2026

Deployment failed with the following error:

Invalid request: `attribution.gitUser` should NOT have additional property `isBot`.

@indietyp indietyp force-pushed the bm/be-494-hashql-scc-loop-breaker-inlining branch from 933ed9e to 1bebdcc Compare April 29, 2026 15:32
@indietyp indietyp force-pushed the bm/be-482-hashql-remove-logical-not-from-unary-operators branch from 68d7a83 to 1685178 Compare April 29, 2026 15:40
@indietyp indietyp force-pushed the bm/be-494-hashql-scc-loop-breaker-inlining branch 2 times, most recently from 1cba715 to 1da0f0a Compare April 29, 2026 15:42
@indietyp indietyp marked this pull request as ready for review April 29, 2026 15:42
@cursor
Copy link
Copy Markdown

cursor Bot commented Apr 29, 2026

PR Summary

Medium Risk
Refactors core inlining control flow around SCC processing and introduces new cycle-detection/ordering logic, which can change generated MIR and inlining behavior across many functions. Risk is mainly correctness/performance regressions in optimization pipelines rather than security.

Overview
Adds a new iterative three-color DFS utility (TriColorDepthFirstSearch + TriColorVisitor) to hashql_core::graph::algorithms, including cycle detection and postorder callbacks, with comprehensive unit tests.

Updates the MIR inliner to handle mutually recursive SCCs via loop-breaker selection: it scores SCC members, greedily chooses a breaker set that makes the remainder acyclic (using the new DFS), reorders SCC members for processing, and changes both normal and aggressive inlining to inline within SCCs except when targeting selected breakers (and to skip self-calls). This also extends Tarjan SCC member utilities with iterators, adds CallGraph::callers() to support scoring, and introduces InlineLoopBreakerConfig (exported via transform prelude) plus new/updated inline tests and snapshots.

Reviewed by Cursor Bugbot for commit 4ea12a3. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread libs/@local/hashql/core/src/graph/algorithms/color/mod.rs
Comment thread libs/@local/hashql/core/src/graph/algorithms/color/mod.rs
@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Apr 29, 2026

🤖 Augment PR Summary

Summary: Adds loop-breaker-aware inlining for mutually recursive HashQL MIR functions, enabling safe inlining within recursive SCCs without infinite expansion.

Changes:

  • Introduces an iterative three-color DFS utility (with cycle detection and postorder callbacks) in hashql_core::graph::algorithms::color.
  • Adds loop-breaker selection (loop_breaker.rs) that scores SCC members and greedily picks a feedback-vertex set to break recursion.
  • Reorders SCC members so non-breakers are processed in postorder of the breaker-removed DAG, followed by breakers, improving within-SCC optimization ordering.
  • Extends the MIR inliner to inline within SCCs except for self-calls and calls targeting loop breakers.
  • Enhances call graph analysis with a callers(def) iterator and SCC member iteration helpers.
  • Updates MIR formatting/tests to render unary NOT as ~ (instead of !) and refreshes snapshots.
  • Adds substantial unit/integration tests covering breaker selection, multi-breaker SCCs, ordering, and aggressive filter behavior.

Technical Notes: Breaker selection uses repeated cycle checks over the remaining subgraph (filtered via DFS edge ignoring) to ensure the non-breaker remainder is acyclic before allowing within-SCC inlining.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread libs/@local/hashql/mir/src/pass/transform/inline/loop_breaker.rs
Comment thread libs/@local/hashql/mir/src/interpret/tests.rs Outdated
@indietyp indietyp force-pushed the bm/be-482-hashql-remove-logical-not-from-unary-operators branch from 84e917d to d7a892b Compare April 30, 2026 08:53
@indietyp indietyp force-pushed the bm/be-494-hashql-scc-loop-breaker-inlining branch from 979bf1e to 3fe9df4 Compare April 30, 2026 08:53
Comment thread libs/@local/hashql/mir/src/pass/transform/inline/find.rs
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4ea12a3. Configure here.


let target_component = self.state.components.scc(ptr);

// Skip if we've already inlined this SCC into this caller.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Aggressive inlining re-expands self-recursive non-breaker functions

Medium Severity

Self-recursive functions in trivial SCCs (size 1 with self-loop) receive no loop breaker because run_in skips SCCs with members.len() < 2. When such a function is called from a filter, the aggressive phase's FindCallsiteVisitor allows repeated inlining: the self-recursive apply @target inside the inlined body passes both checks (ptr == self.caller is false since the caller is the filter, and loop_breakers.contains(ptr) is false). The old SparseBitMatrix-based inlined tracking prevented this by marking the target's SCC after the first expansion; its removal means the aggressive phase re-inlines the same function up to aggressive_inline_cutoff times, causing unnecessary code bloat and a spurious diagnostic.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4ea12a3. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests type/eng > backend Owned by the @backend team

Development

Successfully merging this pull request may close these issues.

2 participants