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

[Fortran] Visit all the symbols and body in the Module and fix other bugs #2747

Merged
merged 25 commits into from
Nov 7, 2023

Conversation

Thirumalai-Shaktivel
Copy link
Member

No description provided.

@Thirumalai-Shaktivel Thirumalai-Shaktivel changed the title [Fortran] Handle variable names staring with underscore and other bug fixes [Fortran] Handle variable names starting with underscore and other bug fixes Oct 27, 2023
@certik
Copy link
Contributor

certik commented Oct 27, 2023

Fixes like these will work most of the time, until they don't -- how will this handle if there is already a variable v__t_1?

@certik
Copy link
Contributor

certik commented Oct 31, 2023

I will review this carefully later, but this approach is good, I think that's the way to do it.

@certik
Copy link
Contributor

certik commented Oct 31, 2023

@Shaikh-Ubaid this is an example of a pass that we might want to run in the --backend=fortran, where we need GFortran to be able to compile it. But we don't want to run it by default, since we want any ASR to be able to print without additional passes.

@Shaikh-Ubaid
Copy link
Member

I think we also need to handle @ in functions from modules, for example:

$ cat examples/expr2.f90 
program expr2
implicit none

print *, repeat("abc", 3)

end program
$ cat pass_27_insert_deallocate.f90 
! Fortran code after applying the pass: insert_deallocate
...
program expr2
implicit none
print *, repeat@repeati32("abc", 3)

end program expr2

@certik
Copy link
Contributor

certik commented Oct 31, 2023

Yes.

I really like this approach of the pass --- that way we are not closing any doors and we can later restrict ASR names if we decide. But right now we don't need to do that, we simply print the ASR as repeat@repeati32("abc", 3) and that's still readable, and if we want the code to compile, we call the pass from this PR, and possibly other passes as needed.

@Thirumalai-Shaktivel
Copy link
Member Author

With the current changes, I get:

Repeat

program expr2
implicit none
print *, repeat("abc", 3)
end program

--show-fortran --dump-all-passes-fortran

! Fortran code after applying the pass: insert_deallocate
module lfortran_intrinsic_iso_fortran_env
implicit none
integer(4), dimension(1), parameter :: character_kinds = [1]
integer(4), parameter :: error_unit = 0
integer(4), parameter :: input_unit = 5
integer(4), parameter :: int16 = 2
integer(4), parameter :: int32 = 4
integer(4), parameter :: int64 = 8
integer(4), parameter :: int8 = 1
integer(4), dimension(2), parameter :: integer_kinds = [4, 8]
integer(4), dimension(1), parameter :: logical_kinds = [4]
integer(4), parameter :: output_unit = 6
integer(4), parameter :: real128 = -1
integer(4), parameter :: real32 = 4
integer(4), parameter :: real64 = 8
integer(4), dimension(2), parameter :: real_kinds = [4, 8]
end module lfortran_intrinsic_iso_fortran_env

module lfortran_intrinsic_builtin
implicit none
end module lfortran_intrinsic_builtin

module lfortran_intrinsic_string
use lfortran_intrinsic_iso_fortran_env, only: int64
implicit none
interface repeat
    module procedure repeati32
end interface repeat

contains

elemental function len_repeati32(n) result(r)
    integer(4), intent(in) :: n
    integer(4) :: r
    r = n
end function len_repeati32
function repeati32(s, n) result(r)
    integer(4) :: i
    integer(4) :: i1
    integer(4), intent(in) :: n
    character(len=len_repeati32(n), kind=1) :: r
    character(len=1, kind=1), intent(in) :: s
    i1 = 1
    i = 1 - 1
    do while (i + 1 <= n)
        i = i + 1
        r(i:i) = s(i1:i1)
    end do
end function repeati32
end module lfortran_intrinsic_string

program expr2
use lfortran_intrinsic_string, only: len_repeati32
use lfortran_intrinsic_string, only: repeat
use lfortran_intrinsic_string, only: repeati32
implicit none
print *, repeat("abc", 3)
end program expr2

MaxVal

program expr2
implicit none
integer :: x(3)
x = 23
end program

--show-fortran --dump-all-passes-fortran --apply-fortran-mangling

! Fortran code after applying the pass: insert_deallocate
program expr2
implicit none
integer(4) :: v__1_t
integer(4) :: v__1_v
integer(4), dimension(3) :: x
v__1_v = lbound(x, 1)
v__1_t = lbound(x, 1) - 1
do while (v__1_t + 1 <= ubound(x, 1))
    v__1_t = v__1_t + 1
    x(v__1_t) = 23
    v__1_v = v__1_v + 1
end do
end program expr2

Both outputs get compiled with gfortran and print the expected output

This PR is ready for review; if there is anything else to be implemented, do let me know.

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.

As long as this mangling is not applied by default, which I think it is not, then this is fine. Thanks!

@certik
Copy link
Contributor

certik commented Nov 4, 2023

Excellent. I think it looks good in general. Go ahead and finish the PR and I give it a final review.

@Thirumalai-Shaktivel
Copy link
Member Author

Thirumalai-Shaktivel commented Nov 4, 2023

We can use git rebase main --committer-date-is-author-date to not change the git commit date; that's a good one.

@Thirumalai-Shaktivel
Copy link
Member Author

Ready for review!

@@ -206,7 +206,7 @@ RUN(NAME include_03 LABELS gfortran llvm wasm c INCLUDE_PATH include_03 fortran)
RUN(NAME cond_01 LABELS gfortran llvm cpp x86 wasm c fortran)
RUN(NAME cond_02 LABELS gfortran llvm wasm c fortran)

RUN(NAME expr_01 FAIL LABELS gfortran llvm cpp x86 wasm c fortran)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is fortran removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we link with the clang, gfortran doesn't generate the stack trace for the failure(It hangs), so I remove it.

@certik
Copy link
Contributor

certik commented Nov 5, 2023

It seems this PR is implementing several unrelated fixes:

  • Improvements to the Fortran backend (however no new tests are added, and old tests are removed)
  • Different way of calling gfortran
  • changes to the frontend's symbol table visitor
  • improvements to the unique symbol pass to handle underscores

Can you split these into separate PRs? Then for each PR we'll make sure tests are added, etc. and we'll merge those that we can merge.

@certik certik marked this pull request as draft November 5, 2023 02:19
@Thirumalai-Shaktivel Thirumalai-Shaktivel force-pushed the asr->fortran_08 branch 2 times, most recently from af00034 to 3cb9ced Compare November 5, 2023 05:15
@Thirumalai-Shaktivel Thirumalai-Shaktivel changed the title [Fortran] Handle variable names starting with underscore and other bug fixes [Fortran] Visit all the symbols in the Module and fix other bugs Nov 5, 2023
@Thirumalai-Shaktivel Thirumalai-Shaktivel changed the title [Fortran] Visit all the symbols in the Module and fix other bugs [Fortran] Visit all the symbols and body in the Module and fix other bugs Nov 5, 2023
@certik
Copy link
Contributor

certik commented Nov 5, 2023

Thanks for splitting it. Now let's add tests for this.

@Thirumalai-Shaktivel
Copy link
Member Author

Thirumalai-Shaktivel commented Nov 7, 2023

Consider the test https://github.com/lfortran/lfortran/blob/main/integration_tests/complex_06.f90, which is already enabled to test the Fortran backend. It imports the lfortran_*.f90 modules inside the program.
All the changes in this PR were introduced because of compiling the lfortran_*.f90 module. So, I think everything is already tested, do you want me to enable other specific tests as well?

cc @certik

@certik
Copy link
Contributor

certik commented Nov 7, 2023

@Thirumalai-Shaktivel go ahead and rebase this and enable Fortran tests that now work.

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 that this looks good to merge, thanks!

@certik certik merged commit 1e865ca into lfortran:main Nov 7, 2023
20 checks passed
@Thirumalai-Shaktivel Thirumalai-Shaktivel deleted the asr->fortran_08 branch February 11, 2024 07:43
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.

None yet

3 participants