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: Using pass_array_by_data() #109

Merged

Conversation

Shaikh-Ubaid
Copy link
Member

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

This PR uses the pass_array_by_data() in the WebAssembly Backend. This enables the wasm backend to pass arrays as arguments to functions.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 8, 2022

As suggested here #109 (comment), I used the pass_unused_functions() at the end. It seems to work to some/good extent. Two of the test cases turned on in TEST: WASM: Turn on tests now pass. Thank you so much @czgdp1807 .

For every test turned on in TEST: WASM: Turn on tests, there is an equivalent test case (for example: for integration_tests/arrays_08_func.f90, there is an equivalent test case integration_tests/arrays_08_func_pass_arr_dims.f90 (already switched on in a previous PR)) in integration_tests/CMakeLists.txt. The equivalent test cases were made to possibly mimic the output of pass_array_by_data().

The test case integration_tests/arrays_08_func.f90 still fails, but its equivalent test case integration_tests/arrays_08_func_pass_arr_dims.f90 passes. I am not sure if it is the WebAsssembly backend that is causing the issue or if it is the integration_tests/arrays_08_func_pass_arr_dims.f90 not being perfect.

Please, could someone possibly share what might be the issue?

@czgdp1807
Copy link
Member

czgdp1807 commented Aug 8, 2022

Well, integration_tests/arrays_08_func.f90 will fail because copy_from_to returns an array as output. And in this PR only arguments with intent(in) (and without allocatable) present are converted to accept array data as output. Arguments with intent(out) need a descriptor to be passed. WASM doesn't support descriptors so it won't work for now. @certik Should I convert array outputs as well to data format in this PR or in a subsequent PR? IMO this is already big enough but still I am fine either way. Let me know.

@czgdp1807
Copy link
Member

czgdp1807 commented Aug 8, 2022

Should I convert array outputs as well to data format in this PR or in a subsequent PR?

I did this anyways and wasn't that tricky. I get the following output. So it seems now there is a bug with pass_unused_function as it isn't removing copy_from_to from the symbol table.

Output

(lf) 19:49:48:~/lfortran_project/lfortran % lfortran --show-wat integration_tests/arrays_08_func.f90
(TranslationUnit 
   (SymbolTable 
      1 
      {
         arrays_08_func: 
            (Program 
               (SymbolTable 
                  2 
                  {
                     copy_from_to: 
                        (Function 
                           (SymbolTable 
                              3 
                              {
                                 a: 
                                    (Variable 
                                       3 
                                       a 
                                       In () () 
                                       Default 
                                       (Integer 4 [
                                          (() ())]) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 b: 
                                    (Variable 
                                       3 
                                       b 
                                       Out () () 
                                       Default 
                                       (Integer 4 [
                                          (() ())]) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 i: 
                                    (Variable 
                                       3 
                                       i 
                                       Local () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.)
                              }) 
                           copy_from_to [
                           (Var 3 a) 
                           (Var 3 b)] [] [
                           (= 
                              (Var 3 i) 
                              (IntegerBinOp (IntegerConstant 1 
                                 (Integer 4 [])) 
                                 Sub (IntegerConstant 1 
                                 (Integer 4 [])) 
                                 (Integer 4 []) ()) ()) 
                           (WhileLoop 
                              (IntegerCompare 
                                 (IntegerBinOp 
                                    (Var 3 i) 
                                    Add (IntegerConstant 1 
                                    (Integer 4 [])) 
                                    (Integer 4 []) ()) 
                                 LtE 
                                 (ArraySize 
                                    (Var 3 a) () 
                                    (Integer 4 []) ()) 
                                 (Integer 4 []) ()) [
                              (= 
                                 (Var 3 i) 
                                 (IntegerBinOp 
                                    (Var 3 i) 
                                    Add (IntegerConstant 1 
                                    (Integer 4 [])) 
                                    (Integer 4 []) ()) ()) 
                              (= 
                                 (ArrayItem 
                                    (Var 3 b) [
                                    (() 
                                       (Var 3 i) ())] 
                                    (Integer 4 []) ()) 
                                 (ArrayItem 
                                    (Var 3 a) [
                                    (() 
                                       (Var 3 i) ())] 
                                    (Integer 4 []) ()) ())])] () 
                           Source 
                           Public 
                           Implementation () .false. .false. .false.), 
                     copy_from_to_a_b: 
                        (Function 
                           (SymbolTable 
                              5 
                              {
                                 1a: 
                                    (Variable 
                                       5 
                                       1a 
                                       In () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 1b: 
                                    (Variable 
                                       5 
                                       1b 
                                       In () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 2a: 
                                    (Variable 
                                       5 
                                       2a 
                                       In () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 2b: 
                                    (Variable 
                                       5 
                                       2b 
                                       In () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 a: 
                                    (Variable 
                                       5 
                                       a 
                                       In () () 
                                       Default 
                                       (Integer 4 [
                                          (
                                             (Var 5 1a) 
                                             (Var 5 2a))]) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 b: 
                                    (Variable 
                                       5 
                                       b 
                                       Out () () 
                                       Default 
                                       (Integer 4 [
                                          (
                                             (Var 5 1b) 
                                             (Var 5 2b))]) 
                                       Source 
                                       Public 
                                       Required .false.), 
                                 i: 
                                    (Variable 
                                       5 
                                       i 
                                       Local () () 
                                       Default 
                                       (Integer 4 []) 
                                       Source 
                                       Public 
                                       Required .false.)
                              }) 
                           copy_from_to_a_b [
                           (Var 5 a) 
                           (Var 5 1a) 
                           (Var 5 2a) 
                           (Var 5 b) 
                           (Var 5 1b) 
                           (Var 5 2b)] [] [
                           (= 
                              (Var 5 i) 
                              (IntegerBinOp (IntegerConstant 1 
                                 (Integer 4 [])) 
                                 Sub (IntegerConstant 1 
                                 (Integer 4 [])) 
                                 (Integer 4 []) ()) ()) 
                           (WhileLoop 
                              (IntegerCompare 
                                 (IntegerBinOp 
                                    (Var 5 i) 
                                    Add (IntegerConstant 1 
                                    (Integer 4 [])) 
                                    (Integer 4 []) ()) 
                                 LtE 
                                 (ArraySize 
                                    (Var 5 a) () 
                                    (Integer 4 []) ()) 
                                 (Integer 4 []) ()) [
                              (= 
                                 (Var 5 i) 
                                 (IntegerBinOp 
                                    (Var 5 i) 
                                    Add (IntegerConstant 1 
                                    (Integer 4 [])) 
                                    (Integer 4 []) ()) ()) 
                              (= 
                                 (ArrayItem 
                                    (Var 5 b) [
                                    (() 
                                       (Var 5 i) ())] 
                                    (Integer 4 []) ()) 
                                 (ArrayItem 
                                    (Var 5 a) [
                                    (() 
                                       (Var 5 i) ())] 
                                    (Integer 4 []) ()) ())])] () 
                           Source 
                           Public 
                           Implementation "" .false. .false. .false.), 
                     i: 
                        (Variable 
                           2 
                           i 
                           Local () () 
                           Default 
                           (Integer 4 []) 
                           Source 
                           Public 
                           Required .false.), 
                  r: 
                     (Variable 
                        2 
                        r 
                        Local () () 
                        Default 
                        (Logical 4 []) 
                        Source 
                        Public 
                        Required .false.), 
               verify_a_b: 
                  (Function 
                     (SymbolTable 
                        6 
                        {
                           1a: 
                              (Variable 
                                 6 
                                 1a 
                                 In () () 
                                 Default 
                                 (Integer 4 []) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           1b: 
                              (Variable 
                                 6 
                                 1b 
                                 In () () 
                                 Default 
                                 (Integer 4 []) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           2a: 
                              (Variable 
                                 6 
                                 2a 
                                 In () () 
                                 Default 
                                 (Integer 4 []) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           2b: 
                              (Variable 
                                 6 
                                 2b 
                                 In () () 
                                 Default 
                                 (Integer 4 []) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           a: 
                              (Variable 
                                 6 
                                 a 
                                 In () () 
                                 Default 
                                 (Integer 4 [
                                    (
                                       (Var 6 1a) 
                                       (Var 6 2a))]) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           b: 
                              (Variable 
                                 6 
                                 b 
                                 In () () 
                                 Default 
                                 (Integer 4 [
                                    (
                                       (Var 6 1b) 
                                       (Var 6 2b))]) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           i: 
                              (Variable 
                                 6 
                                 i 
                                 Local () () 
                                 Default 
                                 (Integer 4 []) 
                                 Source 
                                 Public 
                                 Required .false.), 
                           r: 
                              (Variable 
                                 6 
                                 r 
                                 ReturnVar () () 
                                 Default 
                                 (Logical 4 []) 
                                 Source 
                                 Public 
                                 Required .false.)
                        }) 
                     verify_a_b [
                     (Var 6 a) 
                     (Var 6 1a) 
                     (Var 6 2a) 
                     (Var 6 b) 
                     (Var 6 1b) 
                     (Var 6 2b)] [] [
                     (= 
                        (Var 6 r) 
                        (LogicalConstant .true. 
                           (Logical 4 [])) ()) 
                     (= 
                        (Var 6 i) 
                        (IntegerBinOp (IntegerConstant 1 
                           (Integer 4 [])) 
                           Sub (IntegerConstant 1 
                           (Integer 4 [])) 
                           (Integer 4 []) ()) ()) 
                     (WhileLoop 
                        (IntegerCompare 
                           (IntegerBinOp 
                              (Var 6 i) 
                              Add (IntegerConstant 1 
                              (Integer 4 [])) 
                              (Integer 4 []) ()) 
                           LtE 
                           (ArraySize 
                              (Var 6 a) () 
                              (Integer 4 []) ()) 
                           (Integer 4 []) ()) [
                        (= 
                           (Var 6 i) 
                           (IntegerBinOp 
                              (Var 6 i) 
                              Add (IntegerConstant 1 
                              (Integer 4 [])) 
                              (Integer 4 []) ()) ()) 
                        (= 
                           (Var 6 r) 
                           (LogicalBinOp 
                              (Var 6 r) 
                              And 
                              (IntegerCompare 
                                 (ArrayItem 
                                    (Var 6 a) [
                                    (() 
                                       (Var 6 i) ())] 
                                    (Integer 4 []) ()) 
                                 Eq 
                                 (ArrayItem 
                                    (Var 6 b) [
                                    (() 
                                       (Var 6 i) ())] 
                                    (Integer 4 []) ()) 
                                 (Logical 4 []) ()) 
                              (Logical 4 []) ()) ())])] 
                     (Var 6 r) 
                     Source 
                     Public 
                     Implementation "" .false. .false. .false.), 
               x: 
                  (Variable 
                     2 
                     x 
                     Local () () 
                     Default 
                     (Integer 4 [
                        ((IntegerConstant 1 
                           (Integer 4 [])) (IntegerConstant 10 
                           (Integer 4 [])))]) 
                     Source 
                     Public 
                     Required .false.), 
            y: 
               (Variable 
                  2 
                  y 
                  Local () () 
                  Default 
                  (Integer 4 [
                     ((IntegerConstant 1 
                        (Integer 4 [])) (IntegerConstant 10 
                        (Integer 4 [])))]) 
                  Source 
                  Public 
                  Required .false.)
               }) 
            arrays_08_func [] [
            (= 
               (Var 2 i) 
               (IntegerBinOp (IntegerConstant 1 
                  (Integer 4 [])) 
                  Sub (IntegerConstant 1 
                  (Integer 4 [])) 
                  (Integer 4 []) ()) ()) 
            (WhileLoop 
               (IntegerCompare 
                  (IntegerBinOp 
                     (Var 2 i) 
                     Add (IntegerConstant 1 
                     (Integer 4 [])) 
                     (Integer 4 []) ()) 
                  LtE 
                  (ArraySize 
                     (Var 2 x) () 
                     (Integer 4 []) ()) 
                  (Integer 4 []) ()) [
               (= 
                  (Var 2 i) 
                  (IntegerBinOp 
                     (Var 2 i) 
                     Add (IntegerConstant 1 
                     (Integer 4 [])) 
                     (Integer 4 []) ()) ()) 
               (= 
                  (ArrayItem 
                     (Var 2 x) [
                     (() 
                        (Var 2 i) ())] 
                     (Integer 4 []) ()) 
                  (Var 2 i) ())]) 
            (SubroutineCall 2 copy_from_to_a_b 2 copy_from_to_a_b [
               (
                  (Var 2 x)) 
               ((IntegerConstant 1 
                  (Integer 4 []))) 
               ((IntegerConstant 10 
                  (Integer 4 []))) 
               (
                  (Var 2 y)) 
               ((IntegerConstant 1 
                  (Integer 4 []))) 
               ((IntegerConstant 10 
                  (Integer 4 [])))] ()) 
            (= 
               (Var 2 r) 
               (FunctionCall 2 verify_a_b 2 verify_a_b [
                  (
                     (Var 2 x)) 
                  ((IntegerConstant 1 
                     (Integer 4 []))) 
                  ((IntegerConstant 10 
                     (Integer 4 []))) 
                  (
                     (Var 2 y)) 
                  ((IntegerConstant 1 
                     (Integer 4 []))) 
                  ((IntegerConstant 10 
                     (Integer 4 [])))] 
                  (Logical 4 []) () ()) ()) 
            (Print () [
               (Var 2 r)] () ()) 
            (If 
               (LogicalNot 
                  (Var 2 r) 
                  (Logical 4 []) ()) [
               (ErrorStop ())] [])])
      }) 
   [])
style suggestion: Use '==' instead of '.eq.'
  --> integration_tests/arrays_08_func.f90:29:27
   |
29 |         r = r .and. (a(i) .eq. b(i))
   |                           ^^^^ help: write this as '=='

code generation error: Dimension length for index 0 does not exist


Note: if any of the above error or warning messages are not clear or are lacking
context please report it to us (we consider that a bug that needs to be fixed).

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the czgdp1807_passing_array_explicit_shapes branch from 872dd08 to 7f52662 Compare August 9, 2022 09:55
@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 9, 2022

The test cases arrays_08_func.f90 and arrays_08_func_pass_arr_dims.f90 in integration_tests currently do not pass. Therefore, they are not-turned-on / swtiched-off in this PR.

I am not exactly sure why they do not work. The error I get is

[CompileError: WebAssembly.instantiate(): Compiling function #8 failed: not enough arguments on the stack for local.set, expected 1 more @+481]

From the above error, it seems there are not expected no. of arguments on the top of wasm execution stack. It is possible that the wasm backend might/is not perfect.

There is also another concern (I noticed it previously but did not think it was significant to be solved at that time):

  • the integration_tests sometimes falsely return true/pass for wasm tests.
  • therefore, whenever switching on a test I execute the test and verify the output.

@czgdp1807
Copy link
Member

According to the output in #87 (comment), I think function #8 is _lcompilers_main. So probably WASM backend is doing something wrong for it. And since LLVM backend works so ASR signatures are correct (and on manual inspection of generated ASR as well I feel the same). Can you investigate a little bit more on what's going wrong with WASM backend?

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 9, 2022

Can you investigate a little bit more on what's going wrong with WASM backend?

Yes, working on it.

One of the findings is that all the subroutine integration_tests currently fail for wasm. Most possibly, I did not implement the subroutine logic correctly.

I think, they were falsely passing because of the following.

There is also another concern (I noticed it previously but did not think it was significant to be solved at that time):

the integration_tests sometimes falsely return true/pass for wasm tests.
therefore, whenever switching on a test I execute the test and verify the output.

@czgdp1807
Copy link
Member

czgdp1807 commented Aug 9, 2022

In function #8 why are we accepting one argument. I think _lcompilers_main doesn't need any argument for calling. See,

(func $8 (type 8) (param) (result). Shouldn't it be (func $8 (type 8) () (result).

Ah I see param is just for the indication of arguments (and its not an argument itself). Ignore my comment then.

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the czgdp1807_passing_array_explicit_shapes branch from 7f52662 to 60c75e5 Compare August 9, 2022 10:44
@Shaikh-Ubaid
Copy link
Member Author

Please see #114. It is the wasm backend that fails. I think the pass_array_by_data() works good.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft August 9, 2022 10:57
@certik
Copy link
Contributor

certik commented Aug 9, 2022

Yes, the error Compiling function #8 failed: not enough arguments on the stack for local.set, expected 1 more seems like an error from compiling the WASM code to run in the browser, the local.set didn't have enough arguments on the stack, which can be verified at compile time, indicating that the WASM backend generated incorrect code.

@Shaikh-Ubaid
Copy link
Member Author

Yes, the error Compiling function #8 failed: not enough arguments on the stack for local.set, expected 1 more seems like an error from compiling the WASM code to run in the browser, the local.set didn't have enough arguments on the stack, which can be verified at compile time, indicating that the WASM backend generated incorrect code.

I shared about the error here #114 (comment).

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the czgdp1807_passing_array_explicit_shapes branch from 60c75e5 to 924ee95 Compare August 9, 2022 15:50
@Shaikh-Ubaid Shaikh-Ubaid changed the title WASM: Passing array explicit shapes WASM: Using pass_array_by_data() for passing arrays as arguments Aug 9, 2022
@Shaikh-Ubaid Shaikh-Ubaid changed the title WASM: Using pass_array_by_data() for passing arrays as arguments WASM: Using pass_array_by_data() Aug 9, 2022
@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 9, 2022

Only the last 3 commits are part of this PR. The rest of the commits are part of #114. I will rebase this PR after merging of #114.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review August 9, 2022 15:55
@Shaikh-Ubaid
Copy link
Member Author

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

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the czgdp1807_passing_array_explicit_shapes branch from 924ee95 to 2c58370 Compare August 9, 2022 17:00
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.

Awesome, great job @Shaikh-Ubaid and @czgdp1807!

@certik certik enabled auto-merge August 9, 2022 17:01
@Shaikh-Ubaid
Copy link
Member Author

Great work by @czgdp1807 . Thank you so much for the pass_array_by_data() and thank you for supporting it in the WebAssembly Backend.

@certik certik merged commit 23a54c7 into lfortran:main Aug 9, 2022
@Shaikh-Ubaid Shaikh-Ubaid deleted the czgdp1807_passing_array_explicit_shapes branch August 9, 2022 17:46
@Shaikh-Ubaid Shaikh-Ubaid added wasm WebAssembly Backend arrays Arrays labels Aug 11, 2022
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

3 participants