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

feat: Baseline JIT implementation #29

Merged
merged 10 commits into from
Mar 5, 2019
Merged

feat: Baseline JIT implementation #29

merged 10 commits into from
Mar 5, 2019

Conversation

xxuejie
Copy link
Collaborator

@xxuejie xxuejie commented Feb 20, 2019

NOTE: this PR is quite huge but there are certain files(which contribute to more than half the changes here) we don't need to review:

  • Files in dynasm folder are pulled directly from official LuaJIT, there's no modification by us.
  • src/jit/asm.x64.compiled.c is translated from src/jit/asm.x64.c. Ideally, we might want to exclude this file and only generate it in build steps. But the problem is the translator here is written in Lua, and requires a real Lua binary to run it. As of now, there's no easy way to build a binary, then execute it in build.rs. Hence for simplicity reason, I just included src/jit/asm.x64.compiled.c here, in the future we might want to build a separate crate packaging Lua binary and dynasm translator here to handle this task.

Closes #20

@xxuejie xxuejie mentioned this pull request Feb 20, 2019
2 tasks
@xxuejie xxuejie marked this pull request as ready for review February 21, 2019 02:33
@doitian
Copy link
Member

doitian commented Feb 21, 2019

For src/jit/asm.x64.compiled.c, I think we can add a Makefile rule to compile it using the system Lua to ease maintenance. E.g.

LUA := lua

src/jit/asm.x64.compiled.c: src/jit/x.lua src/jit/asm.x64.c
	$LUA $+

@xxuejie
Copy link
Collaborator Author

xxuejie commented Feb 21, 2019

For src/jit/asm.x64.compiled.c, I think we can add a Makefile rule to compile it using the system Lua to ease maintenance. E.g.

LUA := lua

src/jit/asm.x64.compiled.c: src/jit/x.lua src/jit/asm.x64.c
	$LUA $+

Yeah this would work(and we can actually just submodule LuaJIT this way as well), but my question is: is it common practice to require a Makefile and gcc(yeah I think it's better for us to require gcc and compile Lua as needed instead of requiring system Lua) for a Rust crate? Also that means we won't be able to compile this crate with cargo alone, is that gonna be a concern?

Also, should we choose to go forward with it, this new Makefile-based process is gonna affect CKB as well.

build.rs Outdated Show resolved Hide resolved
@doitian
Copy link
Member

doitian commented Feb 22, 2019

Yeah this would work(and we can actually just submodule LuaJIT this way as well), but my question is: is it common practice to require a Makefile and gcc(yeah I think it's better for us to require gcc and compile Lua as needed instead of requiring system Lua) for a Rust crate? Also that means we won't be able to compile this crate with cargo alone, is that gonna be a concern?

Also, should we choose to go forward with it, this new Makefile-based process is gonna affect CKB as well.

No, the compilation does not depend on GCC and Lua, just keep src/jit/asm.x64.compiled.c in the repo. The command just helps developers to update the file. It also can be added in CI to ensure the two files are synchronized.

@xxuejie
Copy link
Collaborator Author

xxuejie commented Feb 22, 2019

Yeah this would work(and we can actually just submodule LuaJIT this way as well), but my question is: is it common practice to require a Makefile and gcc(yeah I think it's better for us to require gcc and compile Lua as needed instead of requiring system Lua) for a Rust crate? Also that means we won't be able to compile this crate with cargo alone, is that gonna be a concern?
Also, should we choose to go forward with it, this new Makefile-based process is gonna affect CKB as well.

No, the compilation does not depend on GCC and Lua, just keep src/jit/asm.x64.compiled.c in the repo. The command just helps developers to update the file. It also can be added in CI to ensure the two files are synchronized.

This is a very neat idea 👍 , already added Makefile rules and CI target.

src/jit/instructions.rs Outdated Show resolved Hide resolved
src/jit/machine.rs Outdated Show resolved Hide resolved
src/jit/machine.rs Outdated Show resolved Hide resolved
src/jit/machine.rs Show resolved Hide resolved
doitian
doitian previously requested changes Mar 4, 2019

fn reset(&mut self) {
self.asm_data = AsmData::default();
self.rust_data.memory = SparseMemory::<u64>::default();

This comment was marked as resolved.

@doitian doitian dismissed their stale review March 4, 2019 23:58

Wrong comment

@doitian doitian merged commit d8a4eb5 into master Mar 5, 2019
@doitian doitian deleted the baseline-jit branch March 5, 2019 00:09
@xxuejie xxuejie mentioned this pull request Mar 26, 2019
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

Successfully merging this pull request may close these issues.

4 participants