LLVM_WASM: Support testing infrastructure #3866
Conversation
Shaikh-Ubaid
force-pushed
the
llvm_wasm_test_support
branch
2 times, most recently
from
aa44dbf
to
4373243
Compare
|
No tests were removed from |
integration_tests/sin_02.f90
Outdated
| @@ -8,14 +8,14 @@ program sin_02 | |||
| x = 10.5_dp | |||
| r1 = dsin(x) | |||
| r2 = -0.87969575997167_dp | |||
| err = abs(r1-r2) | |||
| err = my_abs(r1-r2) | |||
There was a problem hiding this comment.
The name abs in integration_tests/arrays_04_func_pass_arr_dims.f90 and integration_tests/sin_02.f90 conflicts with abs from math.h when linking with c runtime library which is compiled to wasm. For now I simply renamed the abs to my_abs in these tests.
There was a problem hiding this comment.
This is problematic. If this is needed for the WASM output testing at the CI, then let's patch this file only in the WASM CI test with a patch file, and report a bug.
I would not stop testing this very important use case of a user-defined function with the same name as an intrinsic function.
There was a problem hiding this comment.
I reverted these changes.
conflicts with abs from math.h when linking with c runtime library which is compiled to wasm
The conflict is a warning. So, the tests pass seem to pass. For now, I think it is fine to have the warning. We can later using --mangle with the llvm_wasm backend for these tests. We can also do it in this PR, but since the changes in CMakeLists.txt are related to enabling a backend, I think we should provide such flags in a separate PR.
|
This is ready. |
| @@ -254,7 +254,7 @@ LFORTRAN_API int32_t _lfortran_ichar(char *c); | |||
| LFORTRAN_API int32_t _lfortran_iachar(char *c); | |||
| LFORTRAN_API int32_t _lfortran_all(bool *mask, int32_t n); | |||
| LFORTRAN_API void _lpython_set_argv(int32_t argc_1, char *argv_1[]); | |||
| LFORTRAN_API int32_t _lpython_get_argc(); | |||
| LFORTRAN_API void _lpython_get_argc(int32_t *cnt); | |||
There was a problem hiding this comment.
Why is this change needed?
First, we'll need to do the same change in the LLVM backend, and test it in LPython.
But since we have other functions here that return values, why cannot this one return it?
There was a problem hiding this comment.
But since we have other functions here that return values, why cannot this one return it?
It can return a value. It can also simply take argument by reference and assign the return value to it (current approach). I think it is upto us which approach to follow.
In the LLVM backend, the prototype for this is defined as void _lpython_get_argc(int32_t *cnt) which conflicts with the prototype int32_t _lpython_get_argc(). So, we needed to update one of the prototype to match the other. In this case, I choosed to update the C runtime library prototype so that it matches with the one that we are declaring in the LLVM backend.
There was a problem hiding this comment.
Let me know if we should do the other way round.
There was a problem hiding this comment.
This change breaks LPython. So, I made it the other way round by updating the LLVM backend with the correct prototype. I submitted the change to LPython as well lcompilers/lpython#2650.
integration_tests/sin_02.f90
Outdated
| @@ -8,14 +8,14 @@ program sin_02 | |||
| x = 10.5_dp | |||
| r1 = dsin(x) | |||
| r2 = -0.87969575997167_dp | |||
| err = abs(r1-r2) | |||
| err = my_abs(r1-r2) | |||
There was a problem hiding this comment.
This is problematic. If this is needed for the WASM output testing at the CI, then let's patch this file only in the WASM CI test with a patch file, and report a bug.
I would not stop testing this very important use case of a user-defined function with the same name as an intrinsic function.
.github/workflows/CI.yml
Outdated
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| llvm-version: ["10", "14", "15", "16"] |
There was a problem hiding this comment.
Doesn't this add a lot of tests into our CI and slow it down? Maybe we can test 10 and 16 only?
There was a problem hiding this comment.
Anything is fine for me. I simply used the existing LLVM testing approach at the CI.
Maybe we can test 10 and 16 only?
We can. Let's also test 13 (which is in middle of 10 and 16)?
There was a problem hiding this comment.
For now, I am testing 10 and 16. Hopefully they should be sufficient. If not we can add more later.
|
881 tests pass. Very impressive. |
Shaikh-Ubaid
force-pushed
the
llvm_wasm_test_support
branch
2 times, most recently
from
5829ccb
to
6fc4dae
Compare
|
Ready. |
Shaikh-Ubaid
force-pushed
the
llvm_wasm_test_support
branch
from
6fc4dae
to
218e9e2
Compare
No description provided.