Skip to content

Conversation

ubaidsk
Copy link
Member

@ubaidsk ubaidsk commented Sep 7, 2023

This PR fixes the following issue:

$ cat integration_tests/intrinsics_71.f90      
program intrinsics_71
    implicit none
    integer :: result_value
    result_value = routine(3)

    print *, result_value
    if (result_value /= 600) error stop

contains

    function routine(p) result(r)
        integer, intent(in) :: p
        integer :: a(1, p)
        integer :: b(p, 1)
        integer :: c(1, 1)
        integer :: r

        a(1, 1) = 10
        a(1, 2) = 10
        a(1, 3) = 10

        b(1, 1) = 20
        b(2, 1) = 20
        b(3, 1) = 20

        c = matmul(a, b)
        r = c(1, 1)
    end function routine
end program
$ gfortran integration_tests/intrinsics_71.f90
$ ./a.out 
         600
$ lfortran_in_main integration_tests/intrinsics_71.f90 
ASR verify pass error: ASR verify: The symbol table was not found in the scope of `symtab`.

ASR verify pass error: ASR verify: Var::m_v `p` cannot point outside of its symbol table
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/bin/lfortran.cpp", line 2206
    err = compile_to_object_file(arg_file, tmp_o, false,
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/bin/lfortran.cpp", line 939
    res = fe.get_llvm3(*asr, lpm, diagnostics, infile);
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/lfortran/fortran_evaluator.cpp", line 346
    compiler_options, run_fn, infile);
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 8725
    pass_manager.apply_passes(al, &asr, pass_options, diagnostics);
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/libasr/pass/pass_manager.h", line 293
    apply_passes(al, asr, _passes, pass_options, diagnostics);
  File "/Users/ubaid/Desktop/OpenSource/orig_lfortran/src/libasr/pass/pass_manager.h", line 166
    throw LCompilersException("Verify failed in the pass: "
LCompilersException: Verify failed in the pass: intrinsic_function
$ lfortran integration_tests/intrinsics_71.f90 
600

@ubaidsk ubaidsk force-pushed the fix_intrinsic_func_dep_on_dims branch from 1062701 to 2df6d02 Compare September 7, 2023 05:00
@ubaidsk
Copy link
Member Author

ubaidsk commented Sep 7, 2023

The reference tests seem to be different on Mac and Ubuntu.

@ubaidsk ubaidsk marked this pull request as draft September 7, 2023 05:30
Comment on lines 78 to 83
if( ASR::is_a<ASR::ArrayPhysicalCast_t>(*(*current_expr)) ) {
ASR::ArrayPhysicalCast_t* array_physical_cast_t = ASR::down_cast<ASR::ArrayPhysicalCast_t>(*current_expr);
array_physical_cast_t->m_arg = current_expr_;
} else {
*current_expr = current_expr_;
}
Copy link
Member

Choose a reason for hiding this comment

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

It might not be needed for LFortran but might be needed for LPython. Can you try checking with LPython for this removal?

Copy link
Member

Choose a reason for hiding this comment

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

Or if there is any theoretical explanation that after removing this piece of code, things will still be the same then please provide that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might not be needed for LFortran but might be needed for LPython. Can you try checking with LPython for this removal?

I extracted out the specific code and submitted PRs with this particular change #2387 at LFortran and lcompilers/lpython#2315 at LPython. For LPython I simply copied the whole file intrinsic_function.cpp in LFortran and pasted it in LPython (I think LFortran has the newer version of the file).

I think if the tests passed at both, it would be good to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

if there is any theoretical explanation that after removing this piece of code, things will still be the same then please provide that.

Yes, I shared the explanation at #2387

@czgdp1807
Copy link
Member

The reference tests seem to be different on Mac and Ubuntu.

That's most probably because of the difference in the order in which clang and GCC process the function arguments. Might be some improvements needed in LLVM backend. You might have to look into this yourself until we meet the next time.

@ubaidsk ubaidsk force-pushed the fix_intrinsic_func_dep_on_dims branch from 2db3cb9 to 962302d Compare September 8, 2023 00:44
@ubaidsk ubaidsk force-pushed the fix_intrinsic_func_dep_on_dims branch from 962302d to 230625f Compare September 8, 2023 00:55
@ubaidsk
Copy link
Member Author

ubaidsk commented Sep 8, 2023

I updated the description of this issue. This PR is ready.

@ubaidsk ubaidsk marked this pull request as ready for review September 8, 2023 01:20
@ubaidsk ubaidsk requested a review from certik September 8, 2023 01:24
@ubaidsk ubaidsk merged commit c5e665e into lfortran:main Sep 8, 2023
@ubaidsk ubaidsk deleted the fix_intrinsic_func_dep_on_dims branch September 8, 2023 02:45
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