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: Add new AOT mode to replace experimental JIT mode #72

Merged
merged 8 commits into from Jun 20, 2019

Conversation

@xxuejie
Copy link
Member

commented Jun 13, 2019

This PR adds a new AOT mode evolved from previously experimental JIT mode. It works by compiling a RISC-V program directly into x64 assemblies. After that, most of the code can just run in the compiled x64 assembly binary without any overhead.

Benchmark shows that this can further bump VM performance from 6ms measured in assembly interpreter to ~1.5ms(of which ~0.9ms is the actual running time).

With this new AOT mode, we are also removing our old JIT mode which serves its purpose.

@nervos-bot

This comment was marked as outdated.

Copy link

commented Jun 13, 2019

@TheWaWaR is assigned as the chief reviewer

@xxuejie xxuejie force-pushed the xxuejie:aot branch from 2b3362d to dd16572 Jun 13, 2019

@xxuejie xxuejie marked this pull request as ready for review Jun 13, 2019

@xxuejie xxuejie requested a review from nervosnetwork/ckb-code-review as a code owner Jun 13, 2019

@xxuejie xxuejie requested a review from nervosnetwork/ckb-dev Jun 13, 2019

@codecov

This comment was marked as off-topic.

Copy link

commented Jun 13, 2019

Codecov Report

Merging #72 into develop will decrease coverage by 4.42%.
The diff coverage is 77.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #72      +/-   ##
===========================================
- Coverage    92.56%   88.13%   -4.43%     
===========================================
  Files           20       25       +5     
  Lines         3025     4485    +1460     
===========================================
+ Hits          2800     3953    +1153     
- Misses         225      532     +307
Impacted Files Coverage Δ
src/lib.rs 52% <ø> (-7.1%) ⬇️
src/instructions/mod.rs 98.11% <ø> (ø) ⬆️
definitions/src/asm.rs 100% <ø> (ø) ⬆️
src/machine/asm/mod.rs 74.31% <100%> (+1.96%) ⬆️
src/machine/mod.rs 96.34% <100%> (+0.05%) ⬆️
src/machine/aot/aot.x64.c 65.88% <65.88%> (ø)
src/instructions/ast.rs 76.87% <66.66%> (ø)
src/machine/aot/emitter.rs 86.6% <86.6%> (ø)
src/machine/aot/mod.rs 86.87% <86.87%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fb348d...f3deed3. Read the comment docs.

@xxuejie xxuejie force-pushed the xxuejie:aot branch from dd16572 to 6c9a5e5 Jun 14, 2019

@doitian doitian assigned doitian and unassigned TheWaWaR Jun 18, 2019

@doitian
Copy link
Member

left a comment

Review for LabelGatheringMachine

Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/aot.x64.c Outdated
@doitian

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

  • Review LabelGatheringMachine
  • Review AotCompilingMachine
  • Review instructions in aot.x64.c
    • Value::Op1
    • Value::Op2
    • Value::SignOp2
    • Value::Cond
    • Value::Load
@doitian
Copy link
Member

left a comment

Reviewed Compiling Machine Framework

Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/aot.x64.c
Show resolved Hide resolved src/machine/aot/aot.x64.c
Show resolved Hide resolved src/machine/aot/aot.x64.c
Show resolved Hide resolved src/machine/aot/mod.rs Outdated
Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/mod.rs
Show resolved Hide resolved src/machine/aot/emitter.rs
Show resolved Hide resolved src/machine/aot/emitter.rs
Show resolved Hide resolved src/machine/aot/emitter.rs Outdated
@doitian
Copy link
Member

left a comment

I have reviewed the compilation framework and the most op compilation functions. However, I'm not familiar with X86 and RISC-V instruction set, it is hard for me to verify all the ops are compiled into correct X86 ASM. I think we should run the AOT'd code against RISC-V test suites, is this already set up?

) -> c_int;
fn aot_and(c: *mut AotContext, target: u32, a: AotValue, b: AotValue) -> c_int;
fn aot_or(c: *mut AotContext, target: u32, a: AotValue, b: AotValue) -> c_int;
fn aot_not(c: *mut AotContext, target: u32, a: AotValue, is_signed: c_int) -> c_int;

This comment has been minimized.

Copy link
@doitian

doitian Jun 20, 2019

Member
Suggested change
fn aot_not(c: *mut AotContext, target: u32, a: AotValue, is_signed: c_int) -> c_int;
fn aot_not(c: *mut AotContext, target: u32, a: AotValue, logical: c_int) -> c_int;
@xxuejie

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

I think we should run the AOT'd code against RISC-V test suites, is this already set up?

Yes I've actually written the test suite code out and have it all passed locally, but I haven't pushed it to the test suite repo since as soon as I pushed it, it would invalidate all other PRs in this repo. So the idea is once we merge this PR, I will add the missing test suite related code, then we should be all good. Thoughts?

@doitian

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I think we should run the AOT'd code against RISC-V test suites, is this already set up?

Yes I've actually written the test suite code out and have it all passed locally, but I haven't pushed it to the test suite repo since as soon as I pushed it, it would invalidate all other PRs in this repo. So the idea is once we merge this PR, I will add the missing test suite related code, then we should be all good. Thoughts?

lgtm

@xxuejie xxuejie referenced this pull request Jun 20, 2019

Merged

feat: Add aot mode tests #1

@xxuejie xxuejie merged commit 5ba31b9 into nervosnetwork:develop Jun 20, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@xxuejie xxuejie deleted the xxuejie:aot branch Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.