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 array passing with explicit shapes #99

Merged
merged 10 commits into from
Aug 7, 2022

Conversation

Shaikh-Ubaid
Copy link
Member

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

This PR implements the feature described in this comment #98 (comment).

HIghlights:

  • Support 1-D, 2-D and n-Dimensional arrays.
  • Appropriate integration_tests have been added.

@Shaikh-Ubaid Shaikh-Ubaid added wasm WebAssembly Backend arrays Arrays labels Aug 5, 2022
@Shaikh-Ubaid
Copy link
Member Author

Corresponding to tests arrays_03_func, arrays_04_func and arrays_08_func, I added new tests same as them but which also pass the size of the array as arguments to functions. These new tests are named arrays_03_func_pass_arr_dims, arrays_04_func_pass_arr_dims, arrays_08_func_pass_arr_dims respectively. I will be removing these tests once we have #87 complete and being utilized in the wasm backend.

@Shaikh-Ubaid
Copy link
Member Author

This is ready. Please, possibly review and please share feedback.

@certik
Copy link
Contributor

certik commented Aug 5, 2022

You can keep the new tests.

@certik
Copy link
Contributor

certik commented Aug 5, 2022

I think this looks good.

Can you polish the history? Remove those "revert" commits.

Don't merge, I want to get #97 in first.

@Shaikh-Ubaid
Copy link
Member Author

Can you polish the history? Remove those "revert" commits.

Please, could you possibly share if you mean to remove the word revert from the commit message?

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 5, 2022

Can you polish the history? Remove those "revert" commits.

Please, could you possibly share if you mean to remove the word revert from the commit message?

Since, the pass_propagate_arr_dims() was buggy/not-perfect, I thought we probably might/would not want/wish to keep it. Therefore, the reverting of the commits in which it was added thereby removing the pass completely.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 5, 2022

Please, could you possibly share if the pass_propagate_arr_dims() is still to be kept (despite being not-perfect/buggy)?

@Shaikh-Ubaid
Copy link
Member Author

Don't merge, I want to get #97 in first.

Sure, got it.

@certik
Copy link
Contributor

certik commented Aug 5, 2022

Can you merge with the latest main?

@Shaikh-Ubaid
Copy link
Member Author

Can you merge with the latest main?

Got it. I just pushed a commit with this change.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 6, 2022

doubts:

  • Please, could someone possibly share if the revert commits Revert "Pass: Define pass_propagate_arr_dims()" and Revert "Visitor: Define StatementsFirstWalkVisitorVisitor()" are to be preserved or removed?
  • Also, as suggested in WASM: Initial Support for Array Descriptor #98 (comment), I first needed to add support for 1-D arrays with explicit shapes passed and then 2-D and later n-D arrays.
    • While working on 1-D arrays in this PR, I realized that 2-D and n-D arrays could be supported by just adding a loop to compute the required expression for required memory index. So, in this PR, I tried to also support 2-D and n-D arrays.
    • In this PR, we can also just add support for 1-D arrays and we can add support for 2-D and n-D arrays in separate/subsequent PR/PRs. Please, could someone possibly share if the same is to be done.

@Shaikh-Ubaid
Copy link
Member Author

This is ready. Please possibly review and please share feedback.

@certik
Copy link
Contributor

certik commented Aug 6, 2022

Thanks! I'll review soon.

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 to merge as is. You can keep those revert commits, I thought you added some stuff in this PR and then reverted it, but you reverted something that was already in master, so that's fine.

With these explicit-shape arrays in, what else is missing in order to do the Mandelbrot example?

@Shaikh-Ubaid Shaikh-Ubaid merged commit a275e9c into lfortran:main Aug 7, 2022
@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 7, 2022

With these explicit-shape arrays in, what else is missing in order to do the Mandelbrot example?

I tested the followingmandlebrot code from https://gitlab.com/lfortran/lfortran/-/issues/702#note_976371457

program mandelbrot
    implicit none
    integer  , parameter :: rk       = selected_real_kind (9, 99)
    integer  , parameter :: i_max    =  800
    integer  , parameter :: j_max    =  600
    integer  , parameter :: n_max    =  100
    real (rk), parameter :: x_centre = -0.5_rk
    real (rk), parameter :: y_centre =  0.0_rk
    real (rk), parameter :: width    =  4.0_rk
    real (rk), parameter :: height   =  3.0_rk
    real (rk), parameter :: dx_di    =   width / i_max
    real (rk), parameter :: dy_dj    = -height / j_max
    real (rk), parameter :: x_offset = x_centre - 0.5_rk * (i_max + 1) * dx_di
    real (rk), parameter :: y_offset = y_centre - 0.5_rk * (j_max + 1) * dy_dj
    integer :: image(i_max, j_max)
    integer   :: i
    integer   :: j
    integer   :: n
    real (rk) :: x
    real (rk) :: y
    real (rk) :: x_0
    real (rk) :: y_0
    real (rk) :: x_sqr
    real (rk) :: y_sqr

    do j = 1, j_max
      y_0 = y_offset + dy_dj * j
      do i = 1, i_max
        x_0 = x_offset + dx_di * i
        x = 0.0_rk
        y = 0.0_rk
        n = 0
        do
          x_sqr = x ** 2
          y_sqr = y ** 2
          if (x_sqr + y_sqr > 4.0_rk) then
            image(i,j) = 255
            exit
          end if
          if (n == n_max) then
            image(i,j) = 0
            exit
          end if
          y = y_0 + 2.0_rk * x * y
          x = x_0 + x_sqr - y_sqr
          n = n + 1
        end do
      end do
    end do

    print '(a)', 'P2'
    print '(i0, 1x, i0)', i_max, j_max
    print '(i0)', 255
    do j = 1, j_max
      do i = 1, i_max
        print '(i0)', image(i,j)
      end do
    end do
end program mandelbrot

This code works in the wasm backend, when -

  • I change x_sqr = x ** 2 and y_sqr = y ** 2 to x_sqr = x * x and y_sqr = y * y respectively
    • Since currently there is no pow operator support in the wasm backend.
  • Increase the memory size allotted to wasm to 100 pages (previously, it was 10 pages)
    • In the above mandlebrot code, we have a 2-D array named image of dimensions 800 x 600 and therefore of size 800 x 600 x 4 which is equal to 1920000 bytes which seems to (easily) surpass the current alloted size of 10 pages.
$$10 pages = 10 x 64kib (each page is of size 64kib) = 640kib = 65,5360 < 1920000$$

@Shaikh-Ubaid Shaikh-Ubaid deleted the wasm_array_explicit_shapes branch August 7, 2022 04:51
@Shaikh-Ubaid
Copy link
Member Author

I also tried with page sizes 20 and 30.

  • with page size 20, it does not work (the exact error, same as when page size is 10, is [RuntimeError: WebAssembly.instantiate(): data segment is out of bounds]). I guess, this is because-
$$20 pages = 20 x 64kib (each page is of size 64kib) = 1280kib = 1310720 bytes < 1920000$$
  • with page size 30, it works. I guess, this is because-
$$30 pages = 30 x 64kib (each page is of size 64kib) = 1920kib bytes = 1966080 > 1920000$$

@certik
Copy link
Contributor

certik commented Aug 7, 2022

Excellent!

For the x^2, you can probably add a special case in the WASM backend to check if the power is 2, and if so, just do x*x in the backend, that should be easy I think. For everything else just return a compiler error.

The memory size is allocated in JavaScript?

Once this runs in an online demo, we can simply allocate enough size to support these small demos.

This will be the first step, just print it using print.

Then as the next step, we should return the computed image as an array to JavaScript for showing it as an image.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 7, 2022

For the x^2, you can probably add a special case in the WASM backend to check if the power is 2, and if so, just do x*x in the backend, that should be easy I think. For everything else just return a compiler error.

Got it.

The memory size is allocated in JavaScript?

We can do it two/both ways, that is-

  1. We can create/declare the memory in JavaScript and import it into wasm. Or
  2. We can create/declare the memory in wasm and export it to JavaScript.

Currently, we follow the first way. The memory is created here https://github.com/lfortran/lfortran/blob/main/src/libasr/codegen/wasm_assembler.h#L614 and it is imported into wasm here https://github.com/lfortran/lfortran/blob/main/src/libasr/codegen/asr_to_wasm.cpp#L188.

In the above two code lines, we observe that the min no of pages match, but the max no of pages do not seem to be in sync. JavaScript when creating the memory has specified a max no of pages as 100, where as when importing the memory into wasm we see that we mentioned the max no of pages is 10.

Explanation: In asr_to_wasm.cpp the max no of pages is currently dummy, as mentioned here https://github.com/lfortran/lfortran/blob/main/src/libasr/codegen/wasm_assembler.h#L186. It is not being added to the wasm binary generated by LFortran. As per the wasm binary grammar, mentioning just the min no of pages seemed possible, so, I think to get things started/functioning, I implemented support of only specifying the min no of pages.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 7, 2022

I updated/replaced page size with max/min no of pages in the above comment #99 (comment).

Also, I made a PR here #103. It contains more details.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 8, 2022

For the x^2, you can probably add a special case in the WASM backend to check if the power is 2, and if so, just do x*x in the backend, that should be easy I think. For everything else just return a compiler error.

@certik, please, could you possibly share if you mean to support this as/at compile-time or run-time?

It seems supporting it at compile-time might be simple, but I am unsure about run-time. Please, could you possibly share if we could/should implement a simple pow function for integer powers as follows in the run-time library:

integer function pow(x, n) result(r)
    integer, intent(in) :: x, n
    integer :: r, i
    r = 1
    do i = 1 to n
        r = r * x
   end do
end function

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 8, 2022

It seems evaluatingpow operation at compile-time is already/by-default supported (the m_value attribute/data-member of IntegerBinOp_t contains the compile-time value).

I tried supporting run-time pow by emitting instructions for exponent = 2 case. It is not successful yet. There is type-checking for if block which is making its implementation challenging.

Please, could you possibly share if we could/should just import the pow function from JavaScript for now?

@certik
Copy link
Contributor

certik commented Aug 8, 2022

You can import the pow from JavaScript for now

@certik
Copy link
Contributor

certik commented Aug 8, 2022

But it will be slow, so after it works, we'll have to implement our own version. Yes, we need WASM specific runtime library I think. We can probably just implement this in our regular library, and then you call it from the WASM backend, or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays Arrays wasm WebAssembly Backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants