Skip to content
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

bugfix: place_pi_functions now raises error when graph contains loop #796

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

lexbailey
Copy link
Contributor

I've been trying to generate System Verilog code from the RISC-V sail model.
This PR fixes one bug that I've hit so far.

Sail will hang during generation of the pmpCheck function, this is because it does not detect that there is a cycle in the control flow graph, and instead just keeps recursing over the graph, slowly consuming more memory until it cannot allocate any more, or gets OOM-killed.

I'm not sure if my fix is sound (I'm very new to the code base) but it appears that the set of visited nodes was updated quite late (after the recursive call), and so I made it update earlier.

With my fix, Sail now crashes with a sensible error message instead of hanging:

Error:
pmpCheck: control flow graph is not acyclic (node 254 is in cycle)

I don't know the underlying cause of the cycle yet, that might be another bug elsewhere.

@Alasdair
Copy link
Collaborator

The pmpCheck function has a foreach loop, so that's what would create the cycle (although it should be trivially unrolled, so not sure what's going on there).

I don't think that function is doing anything sensible for cyclic graphs anyway so moving the visited check should be safe.

Copy link

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  724 tests ±0    724 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 435 runs  ±0  2 434 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit dfc733c. ± Comparison against base commit e88a5f0.

@lexbailey
Copy link
Contributor Author

ah okay, it's that simple! I'll see if I can figure out why that loop isn't unrolling.
Thanks.

@Alasdair Alasdair merged commit 1b91aec into rems-project:sail2 Nov 26, 2024
3 checks passed
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.

2 participants