Skip to content

Import Global statements #1617

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

Merged
merged 2 commits into from
Apr 1, 2023
Merged

Conversation

Thirumalai-Shaktivel
Copy link
Collaborator

No description provided.

@Thirumalai-Shaktivel Thirumalai-Shaktivel force-pushed the global_syms_03 branch 2 times, most recently from f359618 to 2eec664 Compare March 29, 2023 10:48
@Thirumalai-Shaktivel
Copy link
Collaborator Author

Thirumalai-Shaktivel commented Mar 30, 2023

TODO

  • Enclose all the global statements into a function: <module_name>@global_statements
  • Call the above function using SubroutineCall
  • Global lists initialization has to be handled
  • check multiple import a statements
  • Pass all the tests

Comment on lines +2247 to +2257
if( init_expr && (current_body || ASR::is_a<ASR::List_t>(*type)) &&
(is_runtime_expression || !is_variable_const)) {
ASR::expr_t* v_expr = ASRUtils::EXPR(ASR::make_Var_t(al, loc, v_sym));
cast_helper(v_expr, init_expr, true);
ASR::asr_t* assign = ASR::make_Assignment_t(al, loc, v_expr,
init_expr, nullptr);
current_body->push_back(al, ASRUtils::STMT(assign));
if (current_body) {
current_body->push_back(al, ASRUtils::STMT(assign));
} else if (ASR::is_a<ASR::List_t>(*type)) {
global_init.push_back(al, assign);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed current_body contains true value only when create_add_variable_to_scope (this function) is visited inside the Function or Program nodes. In the module or global scope, current_body would be nullptr.
So, I have added a check for global_lists.
Have I handled this correctly? @czgdp1807

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sense to me.

@Thirumalai-Shaktivel
Copy link
Collaborator Author

This PR is ready for review.

@czgdp1807 czgdp1807 marked this pull request as ready for review April 1, 2023 06:29
@czgdp1807
Copy link
Collaborator

Please resolve the conflicts.

@czgdp1807 czgdp1807 marked this pull request as draft April 1, 2023 06:29
@Thirumalai-Shaktivel Thirumalai-Shaktivel force-pushed the global_syms_03 branch 3 times, most recently from 515bae5 to d83022f Compare April 1, 2023 06:46
@Thirumalai-Shaktivel
Copy link
Collaborator Author

Done!

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as ready for review April 1, 2023 06:46
Comment on lines 7 to 8
l_2: list[i32]
l_2 = populate_lists()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to add an assignment statement for the initializer. Instead of adding a new test, I just used the existing one 😅.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend to have both tests, initialiser and assignment statement for the initialiser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it! I will update the tests.

@@ -206,47 +203,6 @@ void SymbolTable::move_symbols_from_global_scope(Allocator &al,
es->m_parent_symtab = module_scope;
ASR::symbol_t *s = ASRUtils::symbol_get_past_external(a.second);
LCOMPILERS_ASSERT(s);
if (ASR::is_a<ASR::Variable_t>(*s)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All the initializers of list types are moved into the global_initializer, so I removed all the changes which do the same thing.

@czgdp1807 czgdp1807 marked this pull request as draft April 1, 2023 06:59
- Each module contains two functions: `global_statements` and `global_initializer`.
- `global_statements` contains all the global statements in the module.
- `global_initializer` contains the assign statements that are used to initialize the list or other types that cannot be handled in LLVM's global scope.
@certik certik marked this pull request as ready for review April 1, 2023 20:08
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 looks good to me. Thanks @Thirumalai-Shaktivel !

@certik certik enabled auto-merge April 1, 2023 20:11
@certik certik merged commit f8b720c into lcompilers:main Apr 1, 2023
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