diff --git a/docs/progress-notes/changelog.md b/docs/progress-notes/changelog.md index 46753db89..c07624719 100644 --- a/docs/progress-notes/changelog.md +++ b/docs/progress-notes/changelog.md @@ -219,4 +219,5 @@ ## V0.9 Large change to macro expansion and constant propagation The compiler is now much more aggressive in where and how it expands macros and handles expressions at compiler time. - Several places where macros could be incorrectly executed more than once (possibly causing unwanted side effects) have been fixed. -- Fixed bug in size calculation of non-inline stack arrays. Previous behavior was a compiler assert. \ No newline at end of file +- Fixed bug in size calculation of non-inline stack arrays. Previous behavior was a compiler assert. +- Correctly handle `mod` for unsigned numbers. Previous behavior was to treat all inputs as 32-bit signed integers. \ No newline at end of file diff --git a/goalc/compiler/IR.cpp b/goalc/compiler/IR.cpp index 9c908f810..2dfc10973 100644 --- a/goalc/compiler/IR.cpp +++ b/goalc/compiler/IR.cpp @@ -553,6 +553,8 @@ std::string IR_IntegerMath::print() { return fmt::format("udiv {}, {}", m_dest->print(), m_arg->print()); case IntegerMathKind::IMOD_32: return fmt::format("imod {}, {}", m_dest->print(), m_arg->print()); + case IntegerMathKind::UMOD_32: + return fmt::format("umod {}, {}", m_dest->print(), m_arg->print()); case IntegerMathKind::SARV_64: return fmt::format("sarv {}, {}", m_dest->print(), m_arg->print()); case IntegerMathKind::SHLV_64: @@ -589,7 +591,7 @@ RegAllocInstr IR_IntegerMath::to_rai() { } if (m_kind == IntegerMathKind::IDIV_32 || m_kind == IntegerMathKind::IMOD_32 || - m_kind == IntegerMathKind::UDIV_32) { + m_kind == IntegerMathKind::UDIV_32 || m_kind == IntegerMathKind::UMOD_32) { rai.exclude.emplace_back(emitter::RDX); } return rai; @@ -645,8 +647,10 @@ void IR_IntegerMath::do_codegen(emitter::ObjectGenerator* gen, gen->add_instr(IGen::sar_gpr64_u8(get_reg(m_dest, allocs, irec), m_shift_amount), irec); break; case IntegerMathKind::IMUL_32: { + // just a 32-bit multiply, signed/unsigned doesn't affect lower 32 bits of result. auto dr = get_reg(m_dest, allocs, irec); gen->add_instr(IGen::imul_gpr32_gpr32(dr, get_reg(m_arg, allocs, irec)), irec); + // the PS2 sign extends the result even if we used multu. We replicate this here. gen->add_instr(IGen::movsx_r64_r32(dr, dr), irec); } break; case IntegerMathKind::IMUL_64: { @@ -662,6 +666,9 @@ void IR_IntegerMath::do_codegen(emitter::ObjectGenerator* gen, // zero extend, not sign extend to avoid overflow gen->add_instr(IGen::xor_gpr64_gpr64(Register(RDX), Register(RDX)), irec); gen->add_instr(IGen::unsigned_div_gpr32(get_reg(m_arg, allocs, irec)), irec); + // note: this probably needs hardware testing to know for sure if the PS2 actually sign + // extends here or not. Nothing seems to break either way, and PCSX2/Dobie interpreters both + // sign extend, so that seems like the safest option. gen->add_instr(IGen::movsx_r64_r32(get_reg(m_dest, allocs, irec), emitter::RAX), irec); } break; case IntegerMathKind::IMOD_32: { @@ -669,7 +676,13 @@ void IR_IntegerMath::do_codegen(emitter::ObjectGenerator* gen, gen->add_instr(IGen::idiv_gpr32(get_reg(m_arg, allocs, irec)), irec); gen->add_instr(IGen::movsx_r64_r32(get_reg(m_dest, allocs, irec), emitter::RDX), irec); } break; - + case IntegerMathKind::UMOD_32: { + // zero extend, not sign extend to avoid overflow + gen->add_instr(IGen::xor_gpr64_gpr64(Register(RDX), Register(RDX)), irec); + gen->add_instr(IGen::unsigned_div_gpr32(get_reg(m_arg, allocs, irec)), irec); + // see note on udiv, same applies here. + gen->add_instr(IGen::movsx_r64_r32(get_reg(m_dest, allocs, irec), emitter::RDX), irec); + } break; default: ASSERT(false); } diff --git a/goalc/compiler/IR.h b/goalc/compiler/IR.h index c56da37ab..6b37e6716 100644 --- a/goalc/compiler/IR.h +++ b/goalc/compiler/IR.h @@ -201,6 +201,7 @@ enum class IntegerMathKind { SAR_64, SHR_64, IMOD_32, + UMOD_32, OR_64, AND_64, XOR_64, diff --git a/goalc/compiler/compilation/Math.cpp b/goalc/compiler/compilation/Math.cpp index d4ca7cc8a..aa04024b6 100644 --- a/goalc/compiler/compilation/Math.cpp +++ b/goalc/compiler/compilation/Math.cpp @@ -621,7 +621,11 @@ Val* Compiler::compile_mod(const goos::Object& form, const goos::Object& rest, E con.desired_register = emitter::RAX; fenv->constrain(con); - env->emit_ir(form, IntegerMathKind::IMOD_32, result, second); + env->emit_ir(form, + is_singed_integer_or_binteger(first->type()) + ? IntegerMathKind::IMOD_32 + : IntegerMathKind::UMOD_32, + result, second); auto result_moved = env->make_gpr(first->type()); env->emit_ir(form, result_moved, result); diff --git a/test/goalc/source_templates/arithmetic/divide-2.static.gc b/test/goalc/source_templates/arithmetic/divide-2.static.gc index bb6606249..2e2669274 100644 --- a/test/goalc/source_templates/arithmetic/divide-2.static.gc +++ b/test/goalc/source_templates/arithmetic/divide-2.static.gc @@ -1,3 +1,4 @@ (let ((x 30)) - (+ (/ x 10) 4) + ;; setting upper 32 bits should be ignored + (+ (/ (logior #x111111100000000 x) (logior #x1231234400000000 10)) 4) ) \ No newline at end of file diff --git a/test/goalc/source_templates/arithmetic/divide-signs.static.gc b/test/goalc/source_templates/arithmetic/divide-signs.static.gc new file mode 100644 index 000000000..dc0295e1f --- /dev/null +++ b/test/goalc/source_templates/arithmetic/divide-signs.static.gc @@ -0,0 +1,7 @@ + +(_format #t "~X ~X ~X ~X~%" + (/ -10 2) ;; should be -5 + (/ (the uint -10) 2) ;; should use 64-bit shift logical: #x7ffffffffffffffb + (/ -10 3) ;; should be -3 + (/ (the uint -10) 3) ;; should use integer unsigned divide: #x55555552 + ) \ No newline at end of file diff --git a/test/goalc/source_templates/arithmetic/mod-unsigned.static.gc b/test/goalc/source_templates/arithmetic/mod-unsigned.static.gc new file mode 100644 index 000000000..1e0ec1bc3 --- /dev/null +++ b/test/goalc/source_templates/arithmetic/mod-unsigned.static.gc @@ -0,0 +1,3 @@ +(_format #t "~X ~X~%" + (mod -1 10) ;; -1 + (mod (the uint #xffffffff) 10)) \ No newline at end of file diff --git a/test/goalc/test_arithmetic.cpp b/test/goalc/test_arithmetic.cpp index d40bb7e84..8b8ac040d 100644 --- a/test/goalc/test_arithmetic.cpp +++ b/test/goalc/test_arithmetic.cpp @@ -289,3 +289,12 @@ TEST_F(ArithmeticTests, LogicalOperators) { TEST_F(ArithmeticTests, Comparison) { runner->run_static_test(env, testCategory, "signed-int-compare.static.gc", {"12\n"}); } + +TEST_F(ArithmeticTests, DivideSigns) { + runner->run_static_test(env, testCategory, "divide-signs.static.gc", + {"fffffffffffffffb 7ffffffffffffffb fffffffffffffffd 55555552\n0\n"}); +} + +TEST_F(ArithmeticTests, ModUnsigned) { + runner->run_static_test(env, testCategory, "mod-unsigned.static.gc", {"ffffffffffffffff 5\n0\n"}); +} \ No newline at end of file diff --git a/test/offline/readme.md b/test/offline/readme.md new file mode 100644 index 000000000..52668ea0c --- /dev/null +++ b/test/offline/readme.md @@ -0,0 +1,26 @@ +# Offline Reference Test +The offline reference test runs the decompiler on all files that have a corresponding `_REF.gc`, then compiles them. +The test passes if all files compile and all decompiler outputs match the `_REF.gc`. + +The purpose of the offline reference test is: +- To make sure the output of the decompiler can be compiled +- To let us easily see "what source should change, if I changed this type?". This allows us to safely update types without worrying that we forgot to update some other file. + +This test doesn't run as part of CI, so it relies on us running it manually. As a result, from time to time, it can be broken on master. + +## Running the test +Just run `offline-test` in the build directory. It takes about a minute and will display diffs of any files that don't match and compiler errors on the first failing file. + +## What to do if the diff test fails +First, manually read the diff and make sure that it's a good change. + +If so, re-run the `offline-test` program with the `--dump-mode` flag. It will save copies of any differing output in a `failures` folder (make sure this is empty before running). To apply these to the `_REF.gc` files automatically, there's a python script that you can run like this: +``` +cd jak-project/build +python3 ../scripts/update_decomp_reference.py ./failures ../test/decompiler/referenc +``` + +Next, make sure the actual `.gc` files in `goal_src/` are updated, if they need to be. For large changes, this part can be pretty annoying. There is a `update-goal-src.py` script that is helpful for huge changes. + +## What to do if the compile test fails +Ideally we'd make all code compile successfully without any manual changes. But sometimes there's just one function that doesn't work in a big file, and you'd like to get the rest of it. There's a `config.jsonc` file in the `test/offline` folder that lets you identify functions by name to skip compiling in the ref tests.