Skip to content

refactor(codegen-masm): remove petgraph from Linear tactic, add no_std SCC#713

Merged
bitwalker merged 2 commits into0xMiden:nextfrom
radik878:chore/remove-petgraph-midenc-codegen-masm-447
Oct 17, 2025
Merged

refactor(codegen-masm): remove petgraph from Linear tactic, add no_std SCC#713
bitwalker merged 2 commits into0xMiden:nextfrom
radik878:chore/remove-petgraph-midenc-codegen-masm-447

Conversation

@radik878
Copy link
Copy Markdown
Contributor

  • Replace DiGraphMap + kosaraju_scc with a minimal no_std-friendly SimpleDiGraph and local Kosaraju SCC.
  • Remove petgraph from codegen/masm/Cargo.toml.
  • Use FxHashMap and SmallVec; avoid std-only types.

fix #447

Copy link
Copy Markdown
Collaborator

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

Looks great! I'd like to extract the basic digraph data structure so we can use it elsewhere when needed, but we don't need to expand on the APIs you've provided here now. See my other comment for details.

Aside from that, and a few tests to cover the new data structure, it'll be nice to finally close out this issue!


/// Minimal, no_std-friendly directed graph for `Operand` nodes with SCC computation
#[derive(Default)]
struct SimpleDiGraph {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it'd be worthwhile to extract this to hir/src/adt/digraph.rs, and make it generic over the node type, but only provide an implementation of the graph for node types which implement Hash.

We may have other uses for a basic digraph data structure, so it makes sense to provide a common implementation that we can extend as needed.

Other than that, providing some tests for the provided APIs (namely neighbors_outgoing, reverse, and sccs) and we're good to go!

@radik878
Copy link
Copy Markdown
Contributor Author

Looks great! I'd like to extract the basic digraph data structure so we can use it elsewhere when needed, but we don't need to expand on the APIs you've provided here now. See my other comment for details.

Aside from that, and a few tests to cover the new data structure, it'll be nice to finally close out this issue!

Thanks for review, ser, i've made corrections

@bitwalker bitwalker force-pushed the chore/remove-petgraph-midenc-codegen-masm-447 branch from 5b9d3fd to 4432bfe Compare October 17, 2025 16:00
Copy link
Copy Markdown
Collaborator

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

Turns out petgraph now supports no-std builds, so I've modified this PR to use that instead, as well as clean up a few other dep-related things I spotted

@bitwalker bitwalker merged commit da2261e into 0xMiden:next Oct 17, 2025
10 checks passed
@radik878
Copy link
Copy Markdown
Contributor Author

Turns out petgraph now supports no-std builds, so I've modified this PR to use that instead, as well as clean up a few other dep-related things I spotted

Thank you very much for your help

@github-actions github-actions bot mentioned this pull request Nov 5, 2025
@github-actions github-actions bot mentioned this pull request Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove use of petgraph in midenc-codegen-masm

2 participants