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

Fix felt! macro to use Felt::from_u64_unchecked (instead of from_u32) #361

Open
greenhat opened this issue Dec 16, 2024 · 1 comment
Open

Comments

@greenhat
Copy link
Contributor

What

As discovered in #357, we parse u64 constants as two i32 parts on the stack, which makes translating from_u64_unchecked calls to bitcast op non-optimal. So, temporarily, in #357 I switched for felt! macro to use Felt::from_u32.

How

The easiest way is to peek into the next translated op in the frontend, and if it's a Felt::from_u64_unchecked call translate u64 constant as one u64 value on the stack. Alternative would be to allow to alter n previous translated ops, i.e. convert push.i32.i32 to push.u64.

@bitwalker
Copy link
Contributor

@greenhat The reason we were using u64 was so that we could support the full field element range, u32 only supports a small portion of it (maybe fine for our current use cases), but probably will need to switch back to u64.

I think with the new IR, we'd translate calls to Felt::from_u64_unchecked to the hir.constant operation, with a u64 operand, followed by a hir.cast operation to felt type - this would get constant folded to a hir.constant operation with a felt operand, and lowered to MASM as a field element literal, which I believe is what you want. The only thing we need to add is the Folder implementation for hir.cast.

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

No branches or pull requests

2 participants