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] Handle variable names starting with underscore #2806

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

Thirumalai-Shaktivel
Copy link
Member

No description provided.

@certik
Copy link
Contributor

certik commented Nov 5, 2023

Perfect. Is there a test that this now enables to compile that fails in master?

We need to figure out how to test this. One option is to enable this pass in the "fortran" backend by default, which is the one used by the cmake test suite. And then add a test that would otherwise fail, but now works.

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as draft November 5, 2023 16:18
@Thirumalai-Shaktivel
Copy link
Member Author

Thirumalai-Shaktivel commented Nov 7, 2023

Is there a test that this now enables to compile that fails in master?

Yup, for that we have to apply passes. I came across this issue while testing the array_op pass.

All the variables staring with _ are introduced after applying some passes like array_op, intrinsic_function, ...

We need to apply passes for the Fortran backend, right? Currently we don't.
If we have to apply passes, default passes for LLVM would be correct one, right? Or should I skip some passes there?

@certik
Copy link
Contributor

certik commented Nov 7, 2023

We should apply some passes. I think we should apply this pass in the Fortran backend (in --backend=fortran, not in --show-fortran).

Do you want to try enabling it and see if some new test passes? That would be a good test for this PR.

@Thirumalai-Shaktivel
Copy link
Member Author

Consider the following example:

program expr2
implicit none
integer :: x(2)
x = (2+3)*5
end program

I get, --show-fortran

program expr2
implicit none
integer(4), dimension(2) :: x
x = (2 + 3)*5
end program expr2

--backend=fortran --apply-fortran-mangling

program expr2
implicit none
integer(4) :: v__1_t
integer(4) :: v__libasr_created_scalar_auxiliary_variable
integer(4), dimension(2) :: x
v__libasr_created_scalar_auxiliary_variable = (2 + 3)*5
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) = v__libasr_created_scalar_auxiliary_variable
end do
end program expr2

The following also works
--show-fortran --pass=array_op

program expr2
implicit none
integer(4) :: __1_t
integer(4) :: __libasr_created_scalar_auxiliary_variable
integer(4), dimension(2) :: x
__libasr_created_scalar_auxiliary_variable = (2 + 3)*5
do __1_t = lbound(x, 1), ubound(x, 1)
    x(__1_t) = __libasr_created_scalar_auxiliary_variable
end do
end program expr2

@Thirumalai-Shaktivel
Copy link
Member Author

Enabling the passes requires most of the changes from this PR: #2747, so let's merge that and then work on this.

@certik
Copy link
Contributor

certik commented Nov 7, 2023

Ok, go ahead and rebase this.

Important: the --backend=fortran should not be applying the array_op pass. The only passes it should be applying is to rewrite LFortran extensions to standard Fortran, such as templates or unsigned integers.

I think the application of this PR is when we are printing the ASR using --dump-all-passes-fortran, and we want to print it in such a way to be able to compile. I think it should not be on by default (so that we can see exactly what the ASR has), but as an option (if we want to compile with gfortran, let's say).

@Thirumalai-Shaktivel
Copy link
Member Author

Thirumalai-Shaktivel commented Nov 8, 2023

I see. In that case, I think we won't be able to test this PR changes using integration_tests CMakeLists. As I said earlier, the _ symbols are introduced only after applying the passes.

Should I test it using tests.toml?

@Thirumalai-Shaktivel
Copy link
Member Author

Ready for review.

@@ -392,6 +392,9 @@ namespace LCompilers {
apply_default_passes = false;
}

void use_fortran_passes() {
_user_defined_passes.push_back("unique_symbols");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the unique_symbols pass is always applied even without the --apply-fortran-mangling option? Is this only for the --backend-fortran, or also --dump-all-passes-fortran?

Copy link
Member Author

Choose a reason for hiding this comment

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

The unique_symbols passes are applied only when we pass some options like: https://github.com/lfortran/lfortran/pull/2806/files#diff-0b1b8d6c43c5a0e83ce23cda4c34fde853efebc4baa0cb1676e9f4298f82c532R505-R516

It is used by both --show-fortran and --backend=fortran.
--dump-all-passes-fortran dumps all the passes output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I think this is fine then.

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 is fine.

@Thirumalai-Shaktivel
Copy link
Member Author

Thanks for the review!

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as ready for review November 8, 2023 13:12
@Thirumalai-Shaktivel Thirumalai-Shaktivel merged commit da2d4b9 into lfortran:main Nov 8, 2023
20 checks passed
@Thirumalai-Shaktivel Thirumalai-Shaktivel deleted the fortran_02 branch November 8, 2023 13:12
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

2 participants