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

Strip IncRC instructions from SSA for ACIR functions before reaching ACIRgen #6831

Closed
TomAFrench opened this issue Dec 16, 2024 · 2 comments · Fixed by #6893 or #6894
Closed

Strip IncRC instructions from SSA for ACIR functions before reaching ACIRgen #6831

TomAFrench opened this issue Dec 16, 2024 · 2 comments · Fixed by #6893 or #6894
Assignees

Comments

@TomAFrench
Copy link
Member

In ACIR functions we currently allow IncrementRC instructions to sit in the SSA all the way up to ACIRgen where they're eventually discarded. This means that we need to spend time processing them and memory storing them, their (empty) results and callstack entries.

We can instead just discard them at any point after the runtime separation pass so we don't need to consider them at all.

@aakoshh
Copy link
Contributor

aakoshh commented Dec 20, 2024

If I am reading it correctly, Rc instructions can be created in loop_invariants.rs and constant_folding.rs, so if I was to change convert_ssa_instruction to raise treat Rc as unreachable! the removal of these instructions from ACIR runtimes would need to happen potentially twice: once after runtime separation, which would reduce the number of instructions, and once again before ACIR generation, because the constant folding happens towards the end of the passes. Or those two passes would need to be smarter and not insert these instructions unless the runtime is Brillig, which would probably be a better idea, as we wouldn't have to second guess in each pass whether anything could have re-introduced these instructions.

To be clear, you're worried about the fact that they are there taking up extra slots in the instruction vectors; the compiler will force us to consider them anyway as long as they are part of Instructions and we have common SSA passes for both runtimes, right?

I wonder if there would be a different way of handling this issue via #6841, some kind of separation of the instruction set itself, with some phantom types indicating what kind of runtime we're passing over, so that no pass can create such instructions, and then we can be sure that we really don't need to consider them because they cannot exist 🤔

@jfecher
Copy link
Contributor

jfecher commented Dec 20, 2024

Yes, if unconstrained functions are separated before SSA we could have a simple SSA check to see if the function is constrained, and if so avoid inserting any rc instructions to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants