Skip to content

feat: introduce array_passed_in_function_call pass - #6363

Merged
certik merged 11 commits into
lfortran:mainfrom
Pranavchiku:array-section-pointer-2
Feb 23, 2025
Merged

feat: introduce array_passed_in_function_call pass#6363
certik merged 11 commits into
lfortran:mainfrom
Pranavchiku:array-section-pointer-2

Conversation

@Pranavchiku

@Pranavchiku Pranavchiku commented Feb 19, 2025

Copy link
Copy Markdown
Member

Fixes #6329. Fixes #6214.

One integration test fails due to divergence. I'll check it tomorrow morning.

With this PR we transform:

module function_32_mod_array_section_04
contains
subroutine istril(y)
    real(8), intent(inout) :: y(:, :)
    print *, y(2, 2)
    if ( abs(y(2,2) - 3.0) > 1e-8 ) error stop
    y(2, 2) = 4.0

end subroutine
subroutine matprod(y)
    real(8), intent(inout) :: y(:, :)
    call istril(y)
end subroutine 
end module


program array_section_04
    use function_32_mod_array_section_04
    real(8) :: A(5, 3)
    A = 1.0_8
    A(1,2) = 0
    A(2,2) = 3.0
    call matprod(A(1:2,1:2))
    print *, A(2,2)
end program

To

subroutine matprod(y)
    real(8), dimension(:, :), pointer :: y_tmp
    real(8), dimension(:, :), intent(inout), pointer :: y
    if (.true.) then
        allocate(y_tmp(ubound(y, 1), ubound(y, 2)))
        y_tmp = y
    else
        y_tmp => y
    end if
    call istril(y_tmp, lbound(y, 1), ubound(y, 1), lbound(y,&
         2), ubound(y, 2))
    if (.true.) then
        y = y_tmp
    end if
end subroutine matprod

@Pranavchiku Pranavchiku added the PRIMA Related to compiling the PRIMA code label Feb 19, 2025
@certik

certik commented Feb 19, 2025

Copy link
Copy Markdown
Contributor

Important clarification: The above transformation should only happen when istril is PointerArray (contiguous). The way it is written above it also accepts a descriptor, in which case this pass should NOT apply the transformation.

However in LFortran we run the array data pass that transforms istril into a PointerArray, and then indeed we should apply the transformation.

This is important for the example here: https://github.com/lfortran/lfortran/pull/6363/files#r1962325268.

Comment thread src/libasr/pass/array_passed_in_function_call.cpp
@Pranavchiku
Pranavchiku force-pushed the array-section-pointer-2 branch from b1048bb to b4324e4 Compare February 20, 2025 12:18
@Pranavchiku

Copy link
Copy Markdown
Member Author

cobyla_example_2_fortran segfaults, will create MRE and see what is the problem

@Pranavchiku

Copy link
Copy Markdown
Member Author

Phew! This segfault is the preproc segfault wherein removing a line makes the code work.

@Pranavchiku

Copy link
Copy Markdown
Member Author

MRE:

module cobyla_mod2

contains
subroutine cobylb(amat)
real, intent(in) :: amat(:, :) ! AMAT(N, M_LCON
end subroutine

subroutine cobyla()
implicit none
real, allocatable :: amat(:, :)
character(len=1024) :: str

call get_lincon(amat)
print *, "before: ", ubound(amat, 1), ubound(amat, 2)
str = " "
print *, trim(str)
print *, "after: ", ubound(amat, 1), ubound(amat, 2)
call cobylb(amat)
end subroutine cobyla

subroutine get_lincon(amat)

! Outputs
real, intent(out), allocatable :: amat(:, :)
integer, allocatable :: ixu(:)
real :: idmat(9, 9)

allocate(ixu(40), amat(9, 9))
amat = reshape(shape=[9, 9], source=[idmat(:, ixu)])
end subroutine get_lincon

end module cobyla_mod2

program cobyla_exmp
use cobyla_mod2, only : cobyla
implicit none
call cobyla()
end program cobyla_exmp

I think we can shrink it further more

% lfortran b.f90
before:     9    9

after:     4935887    4914972
% gfortran b.f90 && ./a.out
 before:            9           9
 
 after:            9           9

@HarshitaKalani

HarshitaKalani commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

When I comment the trim statement, I get the same issue. So I don't think this is an issue with trim.

$ lfortran a.f90 
before:     9    9
after:     123    202
$ gfortran a.f90 && ./a.out
 before:            9           9
 after:            9           9

Shorter MRE:

module cobyla_mod2

    contains    
    subroutine cobyla()
    implicit none
    real, allocatable :: amat(:, :)
    call get_lincon(amat)
    print *, "before: ", ubound(amat, 1), ubound(amat, 2)
    print *, "after: ", ubound(amat, 1), ubound(amat, 2)
    end subroutine cobyla
    
    subroutine get_lincon(amat)
    
    ! Outputs
    real, intent(out), allocatable :: amat(:, :)
    integer, allocatable :: ixu(:)
    real :: idmat(9, 9)
    
    allocate(ixu(40), amat(9, 9))
    amat = reshape(shape=[9, 9], source=[idmat(:, ixu)])
    end subroutine get_lincon
    
    end module cobyla_mod2
    
    program cobyla_exmp
    use cobyla_mod2, only : cobyla
    implicit none
    call cobyla()
    end program cobyla_exmp

However, reshape might be causing some issue...

@Pranavchiku

Copy link
Copy Markdown
Member Author

This MRE is tricky, not sure why this is happening.

@Pranavchiku

Copy link
Copy Markdown
Member Author

This PR exposes it, bug is present in latest main as well.

@Pranavchiku

Copy link
Copy Markdown
Member Author

opened issue at: #6377

@Pranavchiku

Copy link
Copy Markdown
Member Author

#6378 will fix the issue so waiting for it to be merged.

@Pranavchiku
Pranavchiku force-pushed the array-section-pointer-2 branch from b4324e4 to 97052c4 Compare February 23, 2025 14:41
@Pranavchiku
Pranavchiku force-pushed the array-section-pointer-2 branch from 97052c4 to 8640a10 Compare February 23, 2025 14:42
@Pranavchiku

Copy link
Copy Markdown
Member Author

Thanks @assem2002! On rebasing main after #6378 is merged, we compile PRIMA, let's wait for CI to run.

@Pranavchiku
Pranavchiku marked this pull request as ready for review February 23, 2025 14:57
@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

One more rebase.

Comment thread src/libasr/pass/array_passed_in_function_call.cpp

@certik certik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job. I think this looks good.

Let's resolve the conflicts.

Before we merge, let's test the performance, to ensure we didn't slow down runtime on existing codes.

@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

Here is the SNAP benchmark on my Apple M4, using 169a921 in snap and this PR:

$ make -j8 FORTRAN=lfortran FFLAGS= MPI=no OPENMP=no
$ time ./gsnap ../qasnap/sample/inp out
6.93 real         6.89 user         0.02 sys
$ make -j8 FORTRAN=lfortran FFLAGS="--fast" MPI=no OPENMP=no
$ time ./gsnap ../qasnap/sample/inp out
0.99 real         0.97 user         0.01 sys

And master (11a9330):

7.16 real         7.12 user         0.02 sys
1.00 real         0.98 user         0.01 sys

So the Release mode has no slowdown. Why is the Debug time faster in this PR? The difference is small, but I would think this PR shouldn't make things faster.

But it's good, no slowdown. Let's check one more code.

@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

dftatom, LFortran master; Debug:

$ time make -f Makefile.manual quicktest
8.07 real         7.91 user         0.14 sys

Release:

3.26 real         3.13 user         0.11 sys

This PR, Debug:

8.11 real         7.95 user         0.14 sys

Release:

3.27 real         3.14 user         0.10 sys

@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

Ok, I tried dftatom and SNAP and I don't see any significant slowdown in either code, in neither Debug nor Release modes. So I think we are in good shape.

Let's resolve the conflicts now.

@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

I resolved the conflicts and put in a merge commit, so that we don't lose the benchmarks and the commits that I tested.

@certik

certik commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

Tests pass, I am going to merge it.

@certik
certik merged commit 9d75637 into lfortran:main Feb 23, 2025
@Pranavchiku

Copy link
Copy Markdown
Member Author

Thanks for wrapping this PR! Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PRIMA Related to compiling the PRIMA code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When a temporary has to be created Array passed in chain of function as argument gets corrupted

3 participants