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

Deduplicate intrinsics with side effects when they're under the same predicate #6533

Closed
TomAFrench opened this issue Nov 15, 2024 · 2 comments · Fixed by #6615
Closed

Deduplicate intrinsics with side effects when they're under the same predicate #6533

TomAFrench opened this issue Nov 15, 2024 · 2 comments · Fixed by #6615
Assignees

Comments

@TomAFrench
Copy link
Member

We currently do not deduplicate any intrinics which have side effects. This results in us not deduplicating instrinsics which we should be able to (example given in #6527 where we do not deduplicate Intrinsic::ToRadix calls of the same variable in the same block).

We essentially need to make deduplication of these instructions aware of deduplicate_with_predicate similarly to how we do for other instructions.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Nov 15, 2024
@TomAFrench TomAFrench changed the title Deduplicate intrinsics with side effects if Deduplicate intrinsics with side effects they're under the same predicate Nov 15, 2024
@jfecher
Copy link
Contributor

jfecher commented Nov 15, 2024

We can add the && deduplicate_with_predicate clause but we also need to actually match on them more closely in that case since we still wouldn't want to deduplicate e.g. println(1)

@TomAFrench TomAFrench changed the title Deduplicate intrinsics with side effects they're under the same predicate Deduplicate intrinsics with side effects when they're under the same predicate Nov 18, 2024
@TomAFrench
Copy link
Member Author

I don't think we have any intrinsics which result in foreign calls afaik but agreed in general that we should be careful when deduplicating these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants