Skip to content

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

3 participants