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.

Sign up for GitHub

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

Jump to bottom

b256 using wide ops #5102

Merged
merged 13 commits into from
Sep 25, 2023
Merged

b256 using wide ops #5102

merged 13 commits into from
Sep 25, 2023

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Sep 8, 2023 •
edited
Loading

Description

This PR change b256 bitwise operations implementation to use wide operators.
It does not add any new functionality to b256.

Other changes are related to clippy complaining about some clones.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@xunilrj xunilrj force-pushed the xunilrj/b256_wide_ops branch from 2464990 to 8a991ca Compare September 11, 2023 13:48
@xunilrj xunilrj marked this pull request as ready for review September 11, 2023 14:49
@xunilrj xunilrj requested a review from a team September 11, 2023 14:49
IGI-111
IGI-111 previously approved these changes Sep 12, 2023
View reviewed changes
@IGI-111 IGI-111 requested a review from a team September 12, 2023 11:51
anton-trunov
anton-trunov reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor thing: let's add some const-eval tests for the b256 intrinsics to test/src/e2e_vm_tests/test_programs/should_pass/language/const_inits/src/main.sw

anton-trunov
anton-trunov previously approved these changes Sep 13, 2023
View reviewed changes
JoshuaBatty
JoshuaBatty previously approved these changes Sep 13, 2023
View reviewed changes
@xunilrj xunilrj mentioned this pull request Sep 13, 2023
Closed
19 tasks
@xunilrj xunilrj dismissed stale reviews from JoshuaBatty and anton-trunov via bb722ec September 14, 2023 14:33
@xunilrj xunilrj force-pushed the xunilrj/b256_wide_ops branch 3 times, most recently from 8c967dc to 492e5d2 Compare September 15, 2023 09:09
@xunilrj xunilrj mentioned this pull request Sep 15, 2023
Closed
IGI-111
IGI-111 previously approved these changes Sep 15, 2023
View reviewed changes
@IGI-111 IGI-111 requested review from JoshuaBatty, anton-trunov and a team September 15, 2023 13:32
anton-trunov
anton-trunov previously approved these changes Sep 16, 2023
View reviewed changes
@xunilrj xunilrj dismissed stale reviews from IGI-111 and anton-trunov via 2d7529e September 19, 2023 17:40
@xunilrj xunilrj force-pushed the xunilrj/b256_wide_ops branch 2 times, most recently from 1db777f to 78ed9f0 Compare September 20, 2023 10:36
@xunilrj xunilrj force-pushed the xunilrj/b256_wide_ops branch from e2a6773 to 248b369 Compare September 21, 2023 13:16
@xunilrj
Copy link
Contributor Author

xunilrj commented Sep 21, 2023

Following the explanation at #5121. My suggestion is to merge this PR with the register allocation changes. It solves the non-determinism and performance is on par with what we have previously.

anton-trunov
anton-trunov previously approved these changes Sep 25, 2023
View reviewed changes
sway-core/src/asm_generation/fuel/register_allocator.rs Outdated Show resolved Hide resolved
@xunilrj xunilrj merged commit 9c30219 into master Sep 25, 2023
@xunilrj xunilrj deleted the xunilrj/b256_wide_ops branch September 25, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anton-trunov anton-trunov anton-trunov approved these changes

@tritao tritao tritao approved these changes

@JoshuaBatty JoshuaBatty Awaiting requested review from JoshuaBatty

@IGI-111 IGI-111 Awaiting requested review from IGI-111

Assignees

@xunilrj xunilrj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xunilrj @tritao @JoshuaBatty @anton-trunov @IGI-111