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

Sync libasr with LFortran #2410

Merged
merged 4 commits into from
Nov 11, 2023
Merged

Conversation

czgdp1807
Copy link
Collaborator

@czgdp1807 czgdp1807 commented Nov 8, 2023

@czgdp1807 czgdp1807 marked this pull request as draft November 8, 2023 10:52
@certik
Copy link
Contributor

certik commented Nov 8, 2023

The changes look good at first sight, but I have not carefully reviewed every single one of them.

If you have any doubts about some area, please point me to it.

Otherwise let's get tests to pass and then we'll do a final review.

src/bin/lpython.cpp Outdated Show resolved Hide resolved
Comment on lines 352 to 333
pass_options.verbose = compiler_options.verbose;
pass_options.all_symbols_mangling = compiler_options.all_symbols_mangling;
pass_options.module_name_mangling = compiler_options.module_name_mangling;
pass_options.global_symbols_mangling = compiler_options.global_symbols_mangling;
pass_options.intrinsic_symbols_mangling = compiler_options.intrinsic_symbols_mangling;
pass_options.verbose = compiler_options.po.verbose;
pass_options.all_symbols_mangling = compiler_options.po.all_symbols_mangling;
pass_options.module_name_mangling = compiler_options.po.module_name_mangling;
pass_options.global_symbols_mangling = compiler_options.po.global_symbols_mangling;
pass_options.intrinsic_symbols_mangling = compiler_options.po.intrinsic_symbols_mangling;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, I think we can reuse compiler_options.po here.

@czgdp1807
Copy link
Collaborator Author

I will continue the sync tomorrow.

@@ -3519,7 +3708,8 @@ namespace IntrinsicScalarFunctionRegistry {
{"exp2", {&Exp2::create_Exp2, &Exp2::eval_Exp2}},
{"expm1", {&Expm1::create_Expm1, &Expm1::eval_Expm1}},
{"fma", {&FMA::create_FMA, &FMA::eval_FMA}},
{"floordiv", {&FloorDiv::create_FloorDiv, &FloorDiv::eval_FloorDiv}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBR

@Shaikh-Ubaid
Copy link
Collaborator

I shared some TBR (to be reverted) comments above. Also, I think there are changes related to Symbolic support that need to be reverted.

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 below concern, I think this PR LGTM!
Thank you!

std::vector<llvm::Value*> args = {tmp};
builder->CreateCall(fn, args);
inline void call_lcompilers_free_strings() {
// if (strings_to_be_deallocated.n > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to uncomment this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually this is an incorrect fix. Correct fix should work with both LFortran and LPython.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, let's merge it as is, and then I will submit a new PR fixing them.

width = atoi(format + 1);
if (dot_pos != NULL) {
dot_pos++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gptsarthak can you review the changes here related to the formatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked with a diffchecker and i dont see any difference between this and lfortran main, so it looks good to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good, thanks for the review!

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 think this seems good to me. I don't see any blocker, hopefully I didn't miss anything.

Copy link

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

Thanks for doing this! Looks good to me.

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid 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. It looks good to me!

@czgdp1807 czgdp1807 merged commit f5948d1 into lcompilers:main Nov 11, 2023
13 checks passed
@czgdp1807 czgdp1807 deleted the libasr_sync_05 branch November 11, 2023 18:42
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.

6 participants