Handle IntegerBinOp_t when setting array dimension through a module variable

[kmr-srbh]

fixes #4013

subroutine a(cs)
   use xx
   real, dimension(nx - 1), intent(in) :: cs
end subroutine

is transformed to:

pure integer function __lcompilers_get_nx()
use xx
get_nx = nx
end function

subroutine a(cs)
   use xx
   interface
      pure integer function __lcompilers_get_nx()
   end function
end interface
real, dimension(__lcompilers_get_nx() - 1), intent(in) :: cs
end subroutine

Working

module xx
   integer :: nx = 4
end module xx

subroutine a(cs)
   use xx
   integer, dimension(nx - 1), intent(in) :: cs
   print *, cs
end subroutine

program main
   integer, dimension(4) :: cs
   cs = [1, 2, 3, 4]
   call a(cs)
end program

(base) saurabh-kumar@Awadh:~/Projects/System/lfortran$ lfortran ./examples/example.f90
1 2 3 

[Pranavchiku]

Do you know how to fix CI?

[kmr-srbh]

Do you know how to fix CI?

I see that the CI failure is due to the SIGSEGV which you pointed out. Let's handle it.

[Pranavchiku]

When you look at logs here https://github.com/lfortran/lfortran/actions/runs/9146050049/job/25145719378?pr=4034#step:6:3508, it states funtions_20 segfaults, go ahead run it locally and try to create minimal code that reproduces same error. Then we can see how to fix this.

[kmr-srbh]

I suspect this part of the code to be the culprit:

program functions_20
real, dimension(3,3) :: A                ! <- variable declared here
real :: B(2)
integer :: k

interface
    function diag_rsp_mat(A, k) result(res)
        real, intent(in) :: A(:,:)           ! <- variable masked here
        integer, intent(in) :: k
        real :: res(minval(shape(A)) - abs(k))
    end function diag_rsp_mat
end interface

From the linter comments, I see that we have a re-declaration occurring here. I am new to the language and currently do not understand what A(:, :) means. @Pranavchiku could you please explain this?

[Pranavchiku]

Does this code give same error when you run it? If yes, seems like you are not checking if end is nullptr or not before processing.

A(:,:) means array of 2 dimension with variable size that will be decided at runtime.

[Pranavchiku]

If the comment above do not work then start adding print statement at each line and check where it segfaults.

[kmr-srbh]

Does this code give same error when you run it? If yes, seems like you are not checking if end is nullptr or not before processing.

A(:,:) means array of 2 dimension with variable size that will be decided at runtime.

Yes it segfaults on my local machine too.

The error occurs on the following line ASR::symbol_t* end_sym = end_var->m_v;. #4034 (comment) can fix it.

[kmr-srbh]

With the latest commit, we now handle the case were both operands are variables. However, unexpected things are happening now. A lot of Illegal instructions (core dumped) errors are happening. @Pranavchiku could you please look into the code to see what is going wrong?

[kmr-srbh]

When running the integration tests, a lot of internal compiler errors pop up. But, running the failing tests one-by-one shows no error and the tests execute properly.

(base) saurabh-kumar@Awadh:~/Projects/System/lfortran$ conda activate lf
(lf) saurabh-kumar@Awadh:~/Projects/System/lfortran$ cd ./integration_tests/
(lf) saurabh-kumar@Awadh:~/Projects/System/lfortran/integration_tests$ lfortran include_02.f90
10 10 10
10 3 10
(lf) saurabh-kumar@Awadh:~/Projects/System/lfortran/integration_tests$ lfortran arrays_op_6.f90
(lf) saurabh-kumar@Awadh:~/Projects/System/lfortran/integration_tests$ lfortran cond_02.f90
ap /= bp
cp /= dp
ap == cp
cp /= dp

[kmr-srbh]

With the changes in this PR, we are now able to compile the module expxs.f90 of the SNAP package.

[kmr-srbh]

This case fails for now integer, dimension(nx * ny - 1), intent(in) :: cs. I am looking into it. We would need to recursively handle this case. What can be a better way to do this? Do we have any ASR utility function for this?

[certik]

Cannot integer, dimension(nx * ny - 1), intent(in) :: cs by handled as integer, dimension(get_cs_dim()), intent(in) :: cs and put nx * ny - 1 into get_cs_dim()?

[kmr-srbh]

Cannot integer, dimension(nx * ny - 1), intent(in) :: cs by handled as integer, dimension(get_cs_dim()), intent(in) :: cs and put nx * ny - 1 into get_cs_dim()?

It can be done. This was my initial thought when implementing the changes in this PR, but I chose to follow the implementation for variables as it was easy to implement taking into account the variables we got in the expression. I think this is the correct way to implement this as then we can handle variables and expressions together with a single implementation. I am pushing the new changes.

[kmr-srbh]

I have a few questions:

1. When handling an expression like nx * ny - 1, how do we know we are dealing with an expression with variables which are an ExternalSymbol? For single variables, it was easier to check it by doing

ASR::Var_t* end_var = ASR::down_cast<ASR::Var_t>(end);
ASR::symbol_t* end_sym = end_var->m_v;
if (ASR::is_a<ASR::ExternalSymbol_t>(*end_sym)) {
    ...
}

We could similarly also handle variables in an InterBinOp_t by checking the operands. How do we handle it here?

2. As we were dealing solely with variables before, we named the new getter function by extracting the symbol name. Now that we deal with expressions as a whole, how do we go about naming it __lcompilers_get_cs_dim() as stated in Handle IntegerBinOp_t when setting array dimension through a module variable #4034 (comment)?

@certik @Pranavchiku could you please guide me on the above? This will help me in completing the new implementation.

[certik]

Right, it would require a more thorough analysis, which might not be worth doing.

It might be easier then to use the get_nx()*get_ny()-1 style. What is the issue in that approach?

[kmr-srbh]

Right, it would require a more thorough analysis, which might not be worth doing.

It might be easier then to use the get_nx()*get_ny()-1 style. What is the issue in that approach?

The issue occurs when dealing with arbitrary number of integer binary operations. When dealing with an expression like nx * ny - 1, we currently check both sides of the operator for a variable. If a variable is found, we convert it to a function call. In this case, we would have nx * ny and 1 as the operands, where one of the operands is again an IntegerBinOp_t. This is not handled and leads to the same issue described in #4013.

We can handle this case through a recursive approach.

[certik]

Yes, I think it has to be handled recursively.

[kmr-srbh]

With the latest commit, we are now able to handle all the cases.

module xx
   integer :: nx = 4
   integer :: ny = 1
end module xx

subroutine a(cs)
   use xx
   integer, dimension(nx * ny - 1), intent(in) :: cs
   print *, cs
end subroutine

program main
   integer, dimension(8) :: cs
   cs = [1, 2, 3, 4, 5, 6, 7, 8]
   call a(cs)
end program

(base) saurabh-kumar@Awadh:~/Projects/System/lfortran$ lfortran ./examples/example.f90
1 2 3 

[kmr-srbh]

Currently, the following integration test fails

program arrays_op_20
implicit none

real, allocatable :: array(:, :), arrayoutput(:, :)

allocate(array(3, 3))
arrayoutput = f(5, array)
print *, size(arrayoutput)
if( size(arrayoutput) /= 24 ) error stop

contains

function f(m, input) result(output)
integer :: m
real :: input(m)
real :: output(2:m, m:2*m)   ! The expression on this line is the cause of the failure
end function

end program

(base) saurabh-kumar@Awadh:~/Projects/System/lfortran$ lfortran ./examples/example.f90
ASR verify pass error: ASR verify: The symbol table was not found in the scope of `symtab`.
  --> ./examples/example.f90:14:18
   |
14 |       integer :: m
   |                  ^ failed here

ASR verify pass error: FunctionCall::m_name `__lcompilers_get_m` cannot point outside of its symbol table
  --> ./examples/example.f90:14:18
   |
14 |       integer :: m
   |                  ^ failed here


Note: Please report unclear, confusing or incorrect messages as bugs at
https://github.com/lfortran/lfortran/issues.

The cause of failure is the declaration real :: output(2:m, m:2*m). This test had passed before, which implies that we do not need to handle this case in this PR. Where am I going wrong?

[certik]

Not sure what is wrong. Save all the ASR to files (--dump-all-passes) and look at them to see when it becomes broken.

[kmr-srbh]

This implementation is now complete.

[certik]

@kmr-srbh beautiful, thank you for pushing this through! It looks ok at first glance to me, but we need to review more carefully.

@Shaikh-Ubaid do you think you could please review it carefully?

[Shaikh-Ubaid]

It looks great to me. Thanks for this! We can improve this further as and when we see/find opportunities.