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

Add unsigned int types #1764

Merged
merged 5 commits into from
May 10, 2023
Merged

Add unsigned int types #1764

merged 5 commits into from
May 10, 2023

Conversation

Smit-create
Copy link
Collaborator

@Smit-create Smit-create requested a review from certik May 8, 2023 11:04
@Smit-create
Copy link
Collaborator Author

@certik Do you think we should move forward with these changes? I wanted your approval before making changes as it will include changes backend as well as some ASR.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

Yes, I think this is the minimal design that will get us there.

This should allow a relatively clean implementation that is separate from the integer implementation and should be easy to maintain. It does add some code everywhere, but it is maintainable for now, so I think we should start with this.

Thanks @Smit-create!

@certik
Copy link
Contributor

certik commented May 9, 2023

This now prints 5:

def main0():
    x: u32
    x = (u32(2)+u32(3))*u32(5)
    print(x)

main0()

# Not implemented yet in LPython:
#if __name__ == "__main__":
#    main()

The ASR is missing the arithmetic expression, it just has 5. But otherwise it seems to work. There are still many bugs and corner cases to get right.

@certik
Copy link
Contributor

certik commented May 10, 2023

Now it works, just constants not generated.

@certik
Copy link
Contributor

certik commented May 10, 2023

Constants work now.

Thorough tests must be added.

@certik certik mentioned this pull request May 10, 2023
6 tasks
@certik
Copy link
Contributor

certik commented May 10, 2023

The added test does not work yet.

@certik
Copy link
Contributor

certik commented May 10, 2023

The test works now.

We need to add many more tests, but the basics are working.

@czgdp1807 can you please review this? I'll polish the history tomorrow and merge.

@certik certik requested a review from czgdp1807 May 10, 2023 05:54
@certik certik marked this pull request as ready for review May 10, 2023 05:54
Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

Approving it but you might have noticed the type generation in LLVM. We need to clean it up and decouple it from ASRToLLVMVisitor. :-).

@certik
Copy link
Contributor

certik commented May 10, 2023

Yes, it's all over the place, it's a mess. Ideally this is done at just once place, and all decisions shifted to physical types to ASR.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

This is good enough now to merge, I polished the commits.

More PRs will be needed with more tests, and fixing corner cases and Debug time wraparound tests.

@czgdp1807 czgdp1807 merged commit 40e7778 into lcompilers:main May 10, 2023
5 checks passed
@Smit-create
Copy link
Collaborator Author

Thanks, @certik for completing it!

@Smit-create Smit-create deleted the i-1588 branch May 11, 2023 11:27
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.

None yet

3 participants