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

LLVM_WASM: Initial support for LLVM->WASM Backend #3842

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

Shaikh-Ubaid
Copy link
Member

towards #3806

add_custom_command(OUTPUT lfortran_runtime_wasm.o
COMMAND $ENV{WASI_SDK_PATH}/bin/clang -I${libasr_SOURCE_DIR}/.. -DCOMPILE_TO_WASM -D_WASI_EMULATED_PROCESS_CLOCKS -c --target=wasm32-wasi ${SRC} -o lfortran_runtime_wasm.o
COMMENT "Cross compiling lfortran_intrinscs.c to lfortran_runtime_wasm.o"
DEPENDS ${SRC})
Copy link
Contributor

@certik certik Apr 9, 2024

Choose a reason for hiding this comment

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

You need to check the output of this and fail if it fails to run. Check status.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it fails currently by default.

@certik
Copy link
Contributor

certik commented Apr 9, 2024

Let's document somewhere how to use this:

Download: https://github.com/WebAssembly/wasi-sdk/releases

diff --git a/build1.sh b/build1.sh
index 49a1cf78b..d2a865215 100755
--- a/build1.sh
+++ b/build1.sh
@@ -3,6 +3,8 @@
 set -e
 set -x
 
+export WASI_SDK_PATH="$HOME/ext/wasi-sdk-21.0"
+
 cmake \
     -DCMAKE_BUILD_TYPE=Debug \
     -DWITH_LLVM=yes \
@@ -12,5 +14,6 @@ cmake \
     -DCMAKE_PREFIX_PATH="$CMAKE_PREFIX_PATH_LFORTRAN;$CONDA_PREFIX" \
     -DCMAKE_INSTALL_PREFIX=`pwd`/inst \
     -DCMAKE_INSTALL_LIBDIR=share/lfortran/lib \
+    -DWITH_TARGET_WASM=yes \
     .
 cmake --build . -j16 --target install

To test this:

$ export WASI_SDK_PATH="$HOME/ext/wasi-sdk-21.0"
$ lfortran --target=wasm32-wasi examples/expr2.f90
25
$ wasmtime expr2.out
25

@certik
Copy link
Contributor

certik commented Apr 9, 2024

Let's give good error message for std::string wasi_sdk_path = std::getenv("WASI_SDK_PATH"); if the path does not exist.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review April 9, 2024 16:56
@Shaikh-Ubaid Shaikh-Ubaid merged commit 8f51765 into lfortran:main Apr 9, 2024
21 checks passed
@Shaikh-Ubaid Shaikh-Ubaid deleted the llvm_wasm branch April 9, 2024 16:59
@Shaikh-Ubaid Shaikh-Ubaid mentioned this pull request Apr 9, 2024
@Pranavchiku Pranavchiku added llvm wasm WebAssembly Backend labels Apr 10, 2024
@@ -26,6 +26,18 @@ target_link_libraries(lfortran_runtime PRIVATE ${MATH_LIBRARIES})
set_target_properties(lfortran_runtime_static PROPERTIES
LIBRARY_OUTPUT_DIRECTORY ..)

if(WITH_TARGET_WASM)
add_custom_command(OUTPUT lfortran_runtime_wasm.o
COMMAND $ENV{WASI_SDK_PATH}/bin/clang -I${libasr_SOURCE_DIR}/.. -DCOMPILE_TO_WASM -D_WASI_EMULATED_PROCESS_CLOCKS -c --target=wasm32-wasi ${SRC} -o lfortran_runtime_wasm.o
Copy link
Contributor

Choose a reason for hiding this comment

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

This was not a good idea!

Neither is the compiler always in bin/ nor is it always called clang.

I also got tripped up by the fact that WITH_TARGET_WASM is a CMake Variable, but WASI_SDK_PATH is an environment variable, very inconsistent.

I think we shouldn't never hard-code paths or compiler names, Debian has a patch about that, too:
https://sources.debian.org/patches/lfortran/0.35.0-1/clang-16.patch/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @junghans. I agree that we should not hard-code paths. I wanted to use the clang compiler from WASI SDK for this specific compilation (or cross-compilation). Rest of the project needs to be built using the normal/regular clang. I attempted looking into how one could compile specific files using a different compiler and it seems CMake does not support it (or I could not find a simple solution for it). The solutions I came across suggested that I need to run CMake twice (that is configure and build the specific files in another execution of CMake). Configuring and building in another CMake execution is possible, but to get started (or for the initial version) I just thought it is better to use an environment variable and hard-code the paths and compile everything in the same execution.

If you know any approach of building some specific files with another compiler using CMake, please do let us know! If you know any other solutions or better approaches, please do share them. I will be happy to update. Thanks for looking into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@junghans is this a blocker for you?

This was a new feature that is not enabled by default, so we got something that works at the CI, and the idea was to iterate on this. Now the next step is to not hardwire anything, but make it configurable. I think specifying LFortran's specific cmake options should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It just makes the fedora package build fail, I will submit a PR to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Shaikh-Ubaid I think the right way to do this would be to use ExternalProject_Add().

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look into it. Thank you for sharing!

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

Successfully merging this pull request may close these issues.

None yet

4 participants