Combine with gbit and pass all the test #15

RinHizakura · 2021-01-20T14:03:10Z

Finally, after this pull request, jitboy can now pass all instruction tests on gbit!

I think I should try to explain some detail here since many small tricks are applied to get over the jitboy's limitation.

Although gbit says that we should reference our emulator's memory to instruction_mem but not copy the contents. It will cause some errors if we directly do this on jitboy. Adding some extra codes may help, but it takes time to do this. So I come up with another solution: When executing the callback function mycpu_set_state in test_cpu.c, it will adjust vm->memory.mem to meet the content that gbit wants. So we can do gbit test correctly although this solution can lead to more executing time.
For jitboy, the implementation of those instructions is not expected to execute one by one, which is how gbit tests the instructions. If we want to force jitboy to do this, we can't always get the correct pc on the JIT codes' return. That's why line 181 in test_cpu.c is there for "faking" the correct pc
In gbit, mymmu_write is expected to be called every time we change the content in memory. But jitboy may sometimes just write memory by jit code and doesn't go through any C code function. To also call mymmu_write for gbit, you can see that some extra write_byte macro will be used.
0xfc and 0xfd are two invalid opcodes in Game Boy, and I borrow them for the status flag's setting and loading. So we can set the state and also retrieve it back as gbit wishes.

How to run gbit

$ make gbit
$ ./build/gbit.out

jserv

Do not copy GBIT source files. Instead, use git submodule and apply necessary patches if any.

jserv

Explain the instruction test in the top-level README.md

Makefile

gbit_src/gbit.c

src/dasm_macros.inc

.gitignore

src/emit.dasc

Makefile

src/memory.h

Makefile

README.md

tests/test_cpu.c

RinHizakura · 2021-01-21T17:29:37Z

Now the code emission for the instruction tester is put in compile_and_run. But you can see that it takes many pointers for returning value. I'll try to find out if there's a better solution.

README.md

src/gbz80.c

tests/test_cpu.h

RinHizakura · 2021-01-23T03:06:50Z

More comments are added for the instruction tester now.

jserv · 2021-01-23T15:10:17Z

Makefile

@@ -19,12 +19,30 @@ else
 endif

 OUT ?= build
-SHELL_HACK := $(shell mkdir -p $(OUT))
+INSTR_TEST_OUT ?= build/tests


Replace INSTR_TEST_OUT ?= build/tests with INSTR_TEST_OUT := $(OUT)/tests because it was not intended to be set directly.

jserv · 2021-01-23T15:11:27Z

README.md

@@ -597,6 +597,29 @@ one of these RAM banks would be chosen to mapped at `0xA000` to `0xBFFF` by MBC,
 state can be restored. When closing jitboy, contents in RAM banks should be copied to the file 
 with the name we mentioned before.

+## Instruction test


Change it to "Instruction compliance tests"

jserv · 2021-01-23T15:14:04Z

README.md

+However, some limitations on jitboy cause problems to integrate with it. So we need to apply 
+some approaches to get over them.
+
+* For jitboy, instructions are not expected to be executed one by one, which is how the tester 


The statement "For jitboy, instructions are not expected to be executed one by one, which is how the tester tests the instructions." is not informative. Instead, you can say, "Due to the JIT compilation, not each instruction is executed at one time."

jserv · 2021-01-23T15:15:26Z

README.md

+write the same value in the same position again.
+* For instructions that write two bytes, another macro `ld16` will be used for `gbz80_mmu_write` 
+calling
+* We borrow `0xfc` and `0xfd` which are the two invalid opcodes in Game Boy for the status 


You should explain how the original opcodes work and why you picked them up.

jserv · 2021-01-23T15:16:25Z

src/gbz80.c

+ * build it at the init stage and then cache it for reusing to avoid spending
+ * time on avoidable codegen.
+ */
+


Don't leave the blank line. The comment should be very close to the corresponding implementation.

jserv · 2021-01-23T15:17:39Z

By the way, we can make use of GitHub Actions for CI/CD after the pull request is merged.

jserv · 2021-01-25T07:38:21Z

Makefile

+$(INSTR_TEST_OUT)/%.o: tests/%.c
+	$(CC) -o $@ -c $(CFLAGS) $< -MMD -MF $@.d
+
+$(INSTR_TEST_OUT)/%.o: src/%.c


I dislike the build rules on src directory. Can you eliminate them? The dependency of the source files in src directory should be already scratched.

I'm confusing about this. Since if this rule is removed, then we can't build the target for the instruction test.

Note that I have separated the object files for jitboy and the instruction tester for the INSTRUCTION_TEST definition, although some of the object files are the same. And this is for if someone executes make all before make check, he/she can still retrieve the correct build target. Please tell me if my approach is not good for solving this problem.

It is not necessary to create a new directory $(INSTR_TEST_OUT). You can use String Substitution to filter-out and replace the names of object files. e.g. build/emit-instr-test.o Of course, you have to generate the corresponding build/emit-instr-test.c

I think that we don't need emit-instr-test.c? Since the file emit.c which is generated by DynASM hasn't been preprocessed, it will be fine to share the same emit.c between jitboy and instruction tester binary?

My idea is to share the built objects as possible as we can. We can benefit from shorter build time and cleaner rules. However, we have to consider the scenario when instruction tester is about to be built right after target "all" is done.

Got it. But now I stuck on trading off between cleaner rules and the ability to share the built objects since I'm not so familiar with Makefile. I'll try to figure out the better approach and asking you to review it after that. 😢

tests/test_cpu.c

jserv · 2021-01-25T07:43:20Z

src/gbz80.h

@@ -11,4 +11,24 @@ bool compile(gb_block *block,

 bool optimize_block(GList **instructions, int opt_level);

+#ifdef INSTRUCTION_TEST
+struct inst_info {


Use instr_info rather than inst_info.
inst is a common term. See: https://en.wikipedia.org/wiki/Inst

jserv · 2021-01-25T09:12:13Z

tests/instr_test.c

+    printf(" -h, --help             Show this help.\n");
+}
+
+static int parse_args(int argc, char **argv)


Use bool instead of plain int.

jserv · 2021-01-25T09:13:18Z

tests/instr_test.c

+    build_load_flag_block(load_flag_block);
+}
+
+void gbz80_close()


Declare this function as "static."

jserv · 2021-01-25T09:14:12Z

src/gbz80.c

+    gbz80_inst *inst = g_new(gbz80_inst, 1);
+
+    uint8_t opcode = mem->mem[pc];
+    bool iscb = false;


is_cb would be better for readability.

jserv · 2021-01-25T09:15:25Z

src/gbz80.c

+    GList *instructions = NULL;
+
+    /* run the fake instruction for flag setting */
+    gbz80_inst *inst_before = g_new(gbz80_inst, 1);


Later, you can submit a new pull request for gbz80_instr name scheme consistency. It was my fault.

jserv · 2021-01-25T09:16:59Z

tests/instr_test.c

+static gb_vm *vm = NULL;
+
+/* since jitboy only update pc when doing instruction jmp/call
+ * so we trickly use a fake one and count it independently to pass gbit


The wording "a fake one" might be misleading. You can say, "introduce an indirect layer for memory/register manipulation."

jserv · 2021-01-25T09:18:18Z

src/dasm_macros.inc

+||    gbz80_mmu_write(addr + 1, (value >> 8) & 0xff);
+|| }
+
+


Cut this blank line.

jserv · 2021-01-25T09:20:20Z

src/gbz80.c

+#ifdef INSTRUCTION_TEST
+  /* To do code generating for flag setting and flag reloading which are 
+   * required for the instruction tester. Opcodes `0xfc` and `0xfd` which 
+   * are invalid opcodes in original Game Boy are borrowed for the purpose */


Replace "borrowed" with "diverted."

jserv · 2021-01-25T09:21:47Z

tests/instr_test.c

+    uint16_t ret = block->func(&vm->state);
+
+    /* Due to the JIT compilation, not each instruction is executed at one time,
+     * which is how the tester tests the instructions. If we want to do this,


Replace "which is how the tester tests the instructions" with "that violates the expectation of instructions tester."

Makefile

jserv · 2021-01-25T16:18:27Z

tests/instr_test.c

+ */
+static uint16_t pc;
+static uint8_t flag;
+/* flag_args is used for carry the arguments for initial status flag


Append a new line for pretty print.

Fix the grammar error in the above comment.

jserv · 2021-01-25T16:18:48Z

tests/instr_test.c

+/* the block for flag loading will be created at the initial and then cached */
+static gb_block *load_flag_block;
+
+


Remove this blank line.

jserv · 2021-01-25T16:21:51Z

src/gbz80.h

@@ -11,4 +11,24 @@ bool compile(gb_block *block,

 bool optimize_block(GList **instructions, int opt_level);

+#ifdef INSTRUCTION_TEST
+struct instr_info {
+    bool iscb;


Rename iscb to is_cb or is_codeblock.

RinHizakura added 3 commits January 20, 2021 20:14

Combine with gbit for instruction test

36d0168

Fix instruction BIT

5ca5c95

Cache jit block for flag loading

c36946f

jserv requested changes Jan 20, 2021

View reviewed changes

Use gbit as git submodule

ae79037

RinHizakura requested a review from jserv January 20, 2021 15:00

jserv requested changes Jan 21, 2021

View reviewed changes