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

Agreing on a impalaJIT version #57

Open
Thomas-Ulrich opened this issue Aug 27, 2024 · 9 comments
Open

Agreing on a impalaJIT version #57

Thomas-Ulrich opened this issue Aug 27, 2024 · 9 comments

Comments

@Thomas-Ulrich
Copy link
Contributor

Thomas-Ulrich commented Aug 27, 2024

which impalaJIT should we use?
probably
https://github.com/ravil-mobile/ImpalaJIT
is the most advanced, but:
https://github.com/SeisSol/easi/blob/master/doc/compiling_dependencies.rst
suggests using https://github.com/uphoffc/ImpalaJIT.git
as well as https://github.com/SeisSol/SeisSol/blob/master/.ci/docker-cpu/Dockerfile.base#L29

@vikaskurapati
Copy link
Contributor

Honestly speaking, I have only used https://github.com/uphoffc/ImpalaJIT.git until now. Is there any reason that we need to switch to https://github.com/ravil-mobile/ImpalaJIT? Performance, more functionalities or otherwise?

@Thomas-Ulrich
Copy link
Contributor Author

@ravil-mobile ?

@davschneller
Copy link
Contributor

davschneller commented Aug 27, 2024

Just to add another suggestion mid-/long-term: we could transpile the ImpalaJIT code to Lua, possibly making ImpalaJIT entirely superfluous (except for potential performance gains, if they exist right now)—while being able to keep the current function maps.

@ravil-mobile
Copy link
Contributor

Honestly speaking, I have only used https://github.com/uphoffc/ImpalaJIT.git until now. Is there any reason that we need to switch to https://github.com/ravil-mobile/ImpalaJIT? Performance, more functionalities or otherwise?

https://github.com/uphoffc/ImpalaJIT.git works only for x86. https://github.com/ravil-mobile/ImpalaJIT generates code for all LLVM backends (e.g., x86, arm, powerpc).

If I were you I would move everything to Lua. It is cumbersome to support yet another code-generator/jit-compiler. Secondly, ImpalaJIT is a small calculator-like language with very restricted type system (used to be a bachelor project). So, Lua is more powerful. Performance-wise, this part of SeisSol is not performance critical (i.e., - executed once per SeisSol invocation). By and large, I would prefer flexibility over performance for this part of the code.

@vikaskurapati
Copy link
Contributor

Honestly speaking, I have only used https://github.com/uphoffc/ImpalaJIT.git until now. Is there any reason that we need to switch to https://github.com/ravil-mobile/ImpalaJIT? Performance, more functionalities or otherwise?

https://github.com/uphoffc/ImpalaJIT.git works only for x86. https://github.com/ravil-mobile/ImpalaJIT generates code for all LLVM backends (e.g., x86, arm, powerpc).

If I were you I would move everything to Lua. It is cumbersome to support yet another code-generator/jit-compiler. Secondly, ImpalaJIT is a small calculator-like language with very restricted type system (used to be a bachelor project). So, Lua is more powerful. Performance-wise, this part of SeisSol is not performance critical (i.e., - executed once per SeisSol invocation). By and large, I would prefer flexibility over performance for this part of the code.

In that case, I would suggest moving to https://github.com/ravil-mobile/ImpalaJIT and mentioning that no new code-generators would be supported and the new function maps should rather be using Lua somewhere in the documentation so that the older function maps still work on the available architectures.

@ravil-mobile
Copy link
Contributor

Honestly speaking, I have only used https://github.com/uphoffc/ImpalaJIT.git until now. Is there any reason that we need to switch to https://github.com/ravil-mobile/ImpalaJIT? Performance, more functionalities or otherwise?

https://github.com/uphoffc/ImpalaJIT.git works only for x86. https://github.com/ravil-mobile/ImpalaJIT generates code for all LLVM backends (e.g., x86, arm, powerpc).
If I were you I would move everything to Lua. It is cumbersome to support yet another code-generator/jit-compiler. Secondly, ImpalaJIT is a small calculator-like language with very restricted type system (used to be a bachelor project). So, Lua is more powerful. Performance-wise, this part of SeisSol is not performance critical (i.e., - executed once per SeisSol invocation). By and large, I would prefer flexibility over performance for this part of the code.

In that case, I would suggest moving to https://github.com/ravil-mobile/ImpalaJIT and mentioning that no new code-generators would be supported and the new function maps should rather be using Lua somewhere in the documentation so that the older function maps still work on the available architectures.

I forgot to mention: https://github.com/ravil-mobile/ImpalaJIT is LLVM based; thus, you would need to compile and install LLVM, which is a heavy dependency for users regarding the compilation time. https://github.com/uphoffc/ImpalaJIT.git is light-weight and takes a second to compile (but restricted only to x86 arch.). I remember that there was a bug in https://github.com/ravil-mobile/ImpalaJIT which I didn't have time to fix (this is related to JIT compilation workflow in LLVM).

@davschneller
Copy link
Contributor

To forward a suggestion: once we're decided, could we add the repo to the SeisSol org maybe? (via creating an ImpalaJIT repo there, and importing (i.e. uploading) the source code)

Besides, feel free to take note of #58 .

@davschneller
Copy link
Contributor

davschneller commented Sep 26, 2024

How do we plan to proceed here? At least there's 2-3 options:

@vikaskurapati
Copy link
Contributor

How do we plan to proceed here? At least there's 2-3 options:

I find the second option better so that we can have one common parser for our material definitions, but we need to ensure that with this, all the older example cases which used the Impala definitions still work. And then, slowly deprecate ImpalaJIT and mention that it would no longer be supported in the upcoming versions of SeisSol in warnings wherever it is used.

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

No branches or pull requests

4 participants