Skip to content

Sync libasr from LFortran #1312

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
Closed
wants to merge 2 commits into from
Closed

Sync libasr from LFortran #1312

wants to merge 2 commits into from

Conversation

czgdp1807
Copy link
Collaborator

No description provided.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Nov 18, 2022
View reviewed changes
src/libasr/utils.h Outdated
Comment on lines 41 to 44
std::string arg_o = "";
bool emit_debug_info = false;
bool emit_debug_line_column = false;
std::string import_path = "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to revert this.

@czgdp1807
Copy link
Collaborator Author

@Thirumalai-Shaktivel I am going through the diff right now. When I will be done I will ping you and others for a second round of inspection. Thanks.

@Thirumalai-Shaktivel
Copy link
Collaborator

Thirumalai-Shaktivel commented Nov 18, 2022 •
edited
Loading

Cool, it seems difficult to sync the changes. Thanks for doing this!

@czgdp1807 czgdp1807 marked this pull request as ready for review November 18, 2022 09:51
@czgdp1807 czgdp1807 mentioned this pull request Nov 18, 2022
Merged
@czgdp1807
Copy link
Collaborator Author

@certik @Shaikh-Ubaid @lucifer1004 @konradha @Pranavchiku @Smit-create @Thirumalai-Shaktivel Please go through the diff and let me know if there's any change that got reverted. I will restore it.

@czgdp1807
Copy link
Collaborator Author

Windows test fail, probably due to the changes in src/libasr/codegen/KaleidoscopeJIT.h. @certik Can you check what needs to be done there?

ubaidsk
ubaidsk approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@ubaidsk ubaidsk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes look fine to me. Thank you so much for this.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Nov 18, 2022
View reviewed changes
Thirumalai-Shaktivel
Thirumalai-Shaktivel approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

Apart from the above change, this looks good to me as well.

Pranavchiku
Pranavchiku approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@Pranavchiku Pranavchiku 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 to me!

Smit-create
Smit-create approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@Smit-create Smit-create left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@certik
Copy link
Contributor

certik commented Nov 18, 2022

I am reviewing it.

@czgdp1807
Copy link
Collaborator Author

Windows test fail. I think its something related to src/libasr/codegen/KaleidoscopeJIT.h. That needs to be worked out. Rest everything is done from my end. @certik I will give you the push access to my fork so that you can make any changes to make Windows tests work.

Also note that WASM shouldn't define functions by topologically sorting them simply because LFortran has some tests where functions depend cyclically on each other and hence they fail. So at least to move things forwarded I reverted to first declaring the functions in WASM backend and then defining them. We need to figure out a general solution for cyclic dependencies later.

certik
certik approved these changes Nov 18, 2022
View reviewed changes
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.

I went over everything, I haven't noticed any issues. Thanks!

@certik
Copy link
Contributor

certik commented Nov 18, 2022

The Windows failures should be fixed in #1314, they are caused by the new LLVM JIT being more strict about what you can do, so some tests that worked with the old JIT do not work anymore.

@certik
Copy link
Contributor

certik commented Nov 18, 2022

Closing in favor of #1314.

@certik certik closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@certik certik certik approved these changes

@ubaidsk ubaidsk ubaidsk approved these changes

@Smit-create Smit-create Smit-create approved these changes

@Thirumalai-Shaktivel Thirumalai-Shaktivel Thirumalai-Shaktivel approved these changes

@Pranavchiku Pranavchiku Pranavchiku approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@czgdp1807 @Thirumalai-Shaktivel @certik @ubaidsk @Smit-create @Pranavchiku