feat: init checks for dependency from current scope implemented by arteevraina · Pull Request #2327 · lfortran/lfortran

arteevraina · 2023-09-03T15:43:58Z

Ref: lcompilers/lpython#2167 (comment)

LPython: lcompilers/lpython#2167

certik · 2023-09-03T15:51:42Z

Thanks! I think you need to resolve some conflicts.

czgdp1807 · 2023-09-04T13:16:23Z

@arteevraina Let's meet on Friday and get it done.

arteevraina · 2023-09-04T13:17:35Z

@arteevraina Let's meet on Friday and get it done.

Sure. @czgdp1807

certik · 2023-09-06T19:17:11Z

I think you need to resolve conflicts for the tests to run.

certik · 2023-09-23T17:09:21Z

@czgdp1807, @arteevraina what's the status of this work? Let's figure out how to wrap this up and finish it.

arteevraina · 2023-09-23T17:32:36Z

@czgdp1807, @arteevraina what's the status of this work? Let's figure out how to wrap this up and finish it.

@certik all of the integration tests are passing now. There are few CI errors which I am figuring out currently and will discuss in my upcoming meeting with @czgdp1807

certik · 2023-09-23T18:51:21Z

Awesome, excellent! Keep me updated and if you need help from me, let me know.

arteevraina · 2023-09-25T06:42:25Z

I had the meeting with @czgdp1807 today and we discussed that there are still some tests that fail other than the integration ones. So, I will try to resolve the issue with those failing tests and run the combination of these commands :
python run_tests.py -u
python run_tests.py -b=llvm -j=1 to check if everything works fine and then hopefully CI should pass.

cc: @certik

certik · 2023-09-25T11:56:14Z

@arteevraina ok. Please ping me if you can't figure out how to finish this.

…function's symbol table

certik · 2023-10-12T03:32:50Z

You have to use LLVM 11. You are updating tests with a later version I think, so they look slightly different.

certik · 2023-10-12T07:38:19Z

src/libasr/asr_utils.h

+    } \
+    if (!is_parent && !ASR::is_a<ASR::ExternalSymbol_t>(*final_sym)) { \
+        current_function_dependencies.push_back(al, dep_name); \
+    } \


Is there a way to write these using functions? Macros are always worse, unless there is no other easy way.

If there is no way using functions, then you need to prefix the macros with LCOMPILERS_* to reduce a chance of collision. In general having macros in header files is a bad idea.

certik · 2023-10-12T07:50:38Z

Overall I think this looks good. @Thirumalai-Shaktivel, @czgdp1807 can you please also review this?

@arteevraina if you could now update your LPython PR to be identical with regards to libasr, that would be great. Then we can merge both at the same time after review.

czgdp1807

LGTM. Just re-write the git history and merge. Thanks for this. A lot of effort went into this.

Thirumalai-Shaktivel

Looks good!

tests/reference/c-program3-32749e4.stdout

tests/reference/asr-functions_06-83a7aca.stdout

ubaidsk · 2023-10-12T10:47:18Z

@arteevraina I appreciate the contributions. I am confused about which specific bug in LFortran this PR fixes. Like we previously discussed, have you had the opportunity to create a minimum possible example such that

it works in gfortran
it fails in lfortran current main branch
this PR fixes it to work with lfortran

certik · 2023-10-12T12:08:00Z

I think this PR and the corresponding PR in LPython fix a bug in ASR that it had incorrect dependencies. The minimal reproducible example is a stricter ASR check, as implemented in this PR. Without the other changes in this PR, tons of tests will fail. So that's why the changes are required.

ubaidsk · 2023-10-12T18:02:24Z

src/libasr/asr_verify.cpp

+                if (dep_sym_global != nullptr && ASRUtils::symbol_parent_symtab(dep_sym_global)->parent == nullptr) {
+                    // This is a global scope symbol, which we allow for now


I think this if condition will never be true or is a dead condition because dep_sym_global will always be nullptr.

We have ASR::symbol_t* dep_sym = sym->resolve_symbol(found_dep); and we are inside if (dep_sym == nullptr) {. Now, we have ASR::symbol_t* dep_sym_global = sym->resolve_symbol(found_dep);. I think dep_sym and dep_sym_global point to the same thing. As dep_sym is nullptr (since we are inside the if), I think dep_sym_globalwould also benullptr`.

Yes, I feel the same thing. If you are planning to search the symbol in parent symbol table, you can try sym->parent->get_symbol(found_dep) and then do sym->parent->resolve_symbol(found_dep) or sym->resolve_symbol(found_dep)

Here, get_symbol searches only in the current symbol table, but resolve_symbol recursively searches in the current, parent, grandparent, ... symbol tables.

src/libasr/asr_verify.cpp

ubaidsk · 2023-10-12T18:15:49Z

src/libasr/asr_verify.cpp

+        // Get the x parent symtab.
+        SymbolTable *sym = x.m_symtab->parent;
+
+        // Dependencies of the function should be from function's parent symbol table.


I am curious to know why dependencies should be from parent's symbol table. Can they be from the grand-parent's /children's symbol table?

What if we call a function from other module? Do we consider it as a dependency? (If we do not consider it as a dependency, please explain why not)

I just read the description at #2327 (comment). How do dependencies work for call to functions from other modules?

You use ExternalSymbol. If it lives in the current symbol table, then you must depend on it. If it lives in the parent/grandparent, then you don't explicitly depend on it.

tests/reference/asr-program3-557dbe8.stdout

certik · 2023-10-18T20:20:11Z

@arteevraina is the LPython PR and the LFortran PR synchronized? Is this ready for final review?

certik · 2023-10-24T11:33:25Z

@czgdp1807 can you please review this?

arteevraina mentioned this pull request Sep 3, 2023

Implemented ASR checks for function dependencies lcompilers/lpython#2167

Merged

arteevraina marked this pull request as draft September 3, 2023 15:49

arteevraina force-pushed the checks-for-same-symtab-dependencies branch 2 times, most recently from a1b42ab to 8ca308d Compare September 13, 2023 05:54

arteevraina force-pushed the checks-for-same-symtab-dependencies branch 3 times, most recently from 50316b9 to 6c4113c Compare September 23, 2023 05:51

arteevraina force-pushed the checks-for-same-symtab-dependencies branch from 6c4113c to 6f61a4e Compare September 30, 2023 03:59

czgdp1807 force-pushed the checks-for-same-symtab-dependencies branch from 4dbf0c0 to 4c63659 Compare October 4, 2023 09:13

arteevraina added 12 commits October 7, 2023 20:33

feat: init checks for dependency from current scope implemented

2705af8

Run asr verify checks

4be6a8a

implement correct dependency checks in instantiate_template

826e525

fix: nullptr exception

d47b38c

fix: integration tests related to associate block

46e0c6d

fix: test block_04.f90

54b6e90

fix: asr verify for checking dependency should not be defined inside …

8b3b579

…function's symbol table

fix: test template_vector'

f2f9871

test: updated test references

b5b565c

fix: log statement

1333182

fix: conflicts & update references

105e1c5

tests: update test references

f4429ad

arteevraina added 2 commits October 12, 2023 11:05

tests: update references using llvm-11

60037ec

Merge branch 'main' into checks-for-same-symtab-dependencies

3b22a9c

arteevraina marked this pull request as ready for review October 12, 2023 06:25

certik requested review from Thirumalai-Shaktivel and czgdp1807 October 12, 2023 07:24

certik reviewed Oct 12, 2023

View reviewed changes

czgdp1807 approved these changes Oct 12, 2023

View reviewed changes

Thirumalai-Shaktivel approved these changes Oct 12, 2023

View reviewed changes

Thirumalai-Shaktivel reviewed Oct 12, 2023

View reviewed changes

tests/reference/c-program3-32749e4.stdout Outdated Show resolved Hide resolved

ubaidsk reviewed Oct 12, 2023

View reviewed changes

tests/reference/asr-functions_06-83a7aca.stdout Show resolved Hide resolved

ubaidsk reviewed Oct 12, 2023

View reviewed changes

src/libasr/asr_verify.cpp Outdated Show resolved Hide resolved

ubaidsk reviewed Oct 12, 2023

View reviewed changes

src/libasr/asr_verify.cpp Outdated Show resolved Hide resolved

ubaidsk reviewed Oct 12, 2023

View reviewed changes

certik reviewed Oct 13, 2023

View reviewed changes

tests/reference/asr-program3-557dbe8.stdout Outdated Show resolved Hide resolved

arteevraina added 4 commits October 18, 2023 14:11

Merge branch 'main' into checks-for-same-symtab-dependencies

a94a307

dev: update logic for update dependencies

51ea508

fix: modern minpack third party compilation failure

043a26b

minor: change variable name to x_symtab

2606075

arteevraina added 2 commits October 24, 2023 13:39

Merge branch 'main' into checks-for-same-symtab-dependencies

9a3adba

tests: update references

738c259

czgdp1807 approved these changes Oct 24, 2023

View reviewed changes

certik merged commit fc9f219 into lfortran:main Oct 24, 2023

arteevraina mentioned this pull request Nov 5, 2023

refactor: checks for dependency in parent_scope in asr_verify #2798

Merged

2 tasks

		if (dep_sym_global != nullptr && ASRUtils::symbol_parent_symtab(dep_sym_global)->parent == nullptr) {
		// This is a global scope symbol, which we allow for now

Uh oh!

Conversation

arteevraina commented Sep 3, 2023 • edited by certik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Sep 3, 2023

Uh oh!

czgdp1807 commented Sep 4, 2023

Uh oh!

arteevraina commented Sep 4, 2023

Uh oh!

certik commented Sep 6, 2023

Uh oh!

certik commented Sep 23, 2023

Uh oh!

arteevraina commented Sep 23, 2023

Uh oh!

certik commented Sep 23, 2023

Uh oh!

arteevraina commented Sep 25, 2023

Uh oh!

certik commented Sep 25, 2023

Uh oh!

certik commented Oct 12, 2023

Uh oh!

certik Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

certik commented Oct 12, 2023

Uh oh!

czgdp1807 left a comment

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ubaidsk commented Oct 12, 2023

Uh oh!

certik commented Oct 12, 2023

Uh oh!

ubaidsk Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ubaidsk Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

ubaidsk Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

certik Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

certik commented Oct 18, 2023

Uh oh!

certik commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arteevraina commented Sep 3, 2023 •

edited by certik

Loading