[goalc] fix mod bug and add div tests (#1296)

* fix mod bug and add div tests

* update changelog

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.
+- 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.

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);
   }

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,

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<IR_IntegerMath>(form, IntegerMathKind::IMOD_32, result, second);
+  env->emit_ir<IR_IntegerMath>(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<IR_RegSet>(form, result_moved, result);

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)
   )

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
+         )

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))

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"});
+}

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.