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

MassaLabs: Eliminate exponentiations in the IR #352

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

Leo-Besancon
Copy link

This PR completes #333, and removes the Operation::Exp node in the IR. Instead, parsed exponentiations are reduced to a series of multiplications using square and multiply.

@bobbinth bobbinth requested review from bitwalker and bobbinth August 8, 2024 05:40
Copy link
Contributor

@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.

LGTM - the clippy errors need to be fixed, but I'll do that in my functions branch, and then we can rebase on top of that to get a green build again

@bitwalker
Copy link
Contributor

@Leo-Besancon I've addressed the clippy warnings in next with the merge of #351, however we have some conflicts, so you'll have to rebase. I don't think there's actually any significant conflicts, just overlapping disjoint changes in the same files, but I haven't checked. Let me know if you have any questions about how to resolve any conflicts you aren't sure about.

Copy link
Contributor

@bobbinth bobbinth 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! Thank you! My only one suggestion would be to modify one of the tests somewhere so that we see how expanding something other than squaring works (e.g., have x^7 somewhere).

@Leo-Besancon Leo-Besancon force-pushed the remove-exponentiation-ir branch from 1c4e707 to 8277fe9 Compare August 9, 2024 07:45
@Leo-Besancon
Copy link
Author

Thank you @bitwalker and @bobbinth, I resolved the small conflicts and updated the complex function test to test the 7 case.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one comment inline - but this is more for the time when we work on the codegen side. Could you create an issue based on it?

@bobbinth bobbinth merged commit 2ed3288 into 0xPolygonMiden:next Aug 9, 2024
4 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.

3 participants