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

WASM: Supporting Multi-Dimensional Arrays #70

Merged
merged 9 commits into from Aug 2, 2022

Conversation

Shaikh-Ubaid
Copy link
Member

@Shaikh-Ubaid Shaikh-Ubaid commented Aug 1, 2022

This PR brings support for multi-dimensional arrays in the wasm Backend.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review August 1, 2022 16:31
@Shaikh-Ubaid
Copy link
Member Author

Example Code for multi-dimensional array:

program expr2
implicit none

integer :: x (4, 10)
integer :: i, j

do i = 1, size(x, dim=1)
    do j = 1, size(x, dim=2)
        x(i, j) = 5
    end do
end do

x(3, 10) = 2

print *, mysum(x)
contains
integer function mysum(a) result(r)
    integer, intent(in) :: a(:, :)
    integer :: i, j
    print *, a(3, 10), a(10, 3)
    r = 0
    do i = 1, size(a, dim=1)
        do j = 1, size(a, dim=2)
            r = r + a(i, j)
        end do
    end do
end function
end program

LLVM Backend Output:

2 5
197

WASM Backend Output:

2 5
197

* integer :: a(:, :)
*
* to:
* integer :: a(2, 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you changing this based on the function call? I don't think you can do that in general, consider, e.g.:

subroutine f(a)
integer :: a(:,:)
end subroutine

integer :: x(2,3)
call f(x)

Then you can indeed "specialize" f for this specific case. But what if you have:

integer :: x(2,3), y(10,11), z(20,21)
call f(x)
call f(y)
call f(z)

?

I think the way to do it is to just inline the function with our inline pass, then that should take care of it.

Alternatively, modify this pass to do the following:

Change this:

subroutine f(a, b)
integer :: a(:,:)
real :: b(:,:,:)
end subroutine

to this

subroutine f(na1, na2, a, nb1, nb2, nb3, b)
integer, intent(in) :: na1, na2, nb1, nb2, nb3
integer :: a(na1, na2)
real :: b(nb1, nb2, nb3)
end subroutine

And you have to change all places that call this function to change from:

call f(x, y)

to

call f(size(x,1), size(x, 2), x, size(y, 1), size(y, 2), size(y, 3), y)

This would be beautiful and extremely useful, and for cases when the array is contiguous, this would be equivalent (I think).

Copy link
Contributor

Choose a reason for hiding this comment

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

Effectively this would pass the array descriptor "by value" already at the ASR level. It would be an optional pass (for now you can use it in the WASM backend), but very useful down the road. These are the kinds of code transformations that we need to have, and later on we will combine them in various ways to get very good optimization pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I will update the pass soon as suggested above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, yes, I think, the current approach would/will fail on the shared example.

Then you can indeed "specialize" f for this specific case. But what if you have:

integer :: x(2,3), y(10,11), z(20,21)
call f(x)
call f(y)
call f(z)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the suggested approach looks beautiful to me as well. Thank you for sharing it.

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, every time I see identical code like this, I get more and more convinced we should just merge Subroutine/Function: lcompilers/lpython#866.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 1, 2022

Ondřej, as currently the pass_propagate_arr_dims() is used by just the wasm backend, please, could you possibly share if we could possibly merge this PR? If we could possibly merge this, I think, I could work on the pass_propagate_arr_dims() iteratively in the subsequent PR/PRs.

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.

Yes, this is fine to merge as is.

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