Fix SRA and SRAV instructions

This implementation is correct one but there is no guarantee that it
will work with all compilers so we should always check on given platform
that tests pass (and potentially fix it).

4 files changed, 50 insertions, 9 deletions

diff --git a/instructions.md b/instructions.md
index f9666ec..4340a7a 100644
--- a/instructions.md
+++ b/instructions.md
@@ -4,7 +4,6 @@ This is list of all MIPS1 instructions and their implementation status in QtMips
 
 Explanation of checkboxes:
 * [ ] Not tested
-* [?] Somewhat tested but not sure about correctness of implementation
 * [x] Tested
 
 CPU Arithmetic Instruction
@@ -98,8 +97,8 @@ CPU Shift Instructions
 ----------------------
 * [x] SLL
 * [x] SLLV
-* [?] SRA
-* [?] SRAV
+* [x] SRA
+* [x] SRAV
 * [x] SRL
 * [x] SRLV
 
diff --git a/qtmips_machine/alu.cpp b/qtmips_machine/alu.cpp
index 0d40bf0..f266c9b 100644
--- a/qtmips_machine/alu.cpp
+++ b/qtmips_machine/alu.cpp
@@ -11,15 +11,15 @@ std::uint32_t machine::alu_operate(enum AluOp operation, std::uint32_t s, std::u
         case ALU_OP_SRL:
             return t >> sa;
         case ALU_OP_SRA:
-            // TODO is this correct implementation? (Should we be masking top most bit?)
-            return ((t & 0x7fffffff) >> sa) | (t & 0x80000000);
+            // Note: This might be broken with some compilers but works with gcc
+            return (std::int32_t)t >> sa;
         case ALU_OP_SLLV:
             return t << s;
         case ALU_OP_SRLV:
             return t >> s;
         case ALU_OP_SRAV:
-            // TODO is this correct implementation? (Should we be masking top most bit?)
-            return ((t & 0x7fffffff) >> s) | (t & 0x80000000);
+            // Note: same note as in case of SRA
+            return (std::int32_t)t >> s;
         case ALU_OP_JR:
         case ALU_OP_JALR:
             // Do nothing as we solve this when we are handling program counter in instruction decode (handle_pc)
diff --git a/qtmips_machine/tests/testalu.cpp b/qtmips_machine/tests/testalu.cpp
index 7668679..a701b64 100644
--- a/qtmips_machine/tests/testalu.cpp
+++ b/qtmips_machine/tests/testalu.cpp
@@ -13,7 +13,49 @@ void MachineTests::alu_data() {
     QTest::addColumn<Registers>("regs_res");
     QTest::addColumn<std::uint32_t>("res");
 
-    // TODO SLL-SRAV
+    QTest::newRow("SLL") << ALU_OP_SLL \
+                         << (std::uint32_t)0 \
+                         << (std::uint32_t)0x80000001 \
+                         << (std::uint8_t)3 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0x8;
+    QTest::newRow("SRL") << ALU_OP_SRL \
+                         << (std::uint32_t)0 \
+                         << (std::uint32_t)0x80000008 \
+                         << (std::uint8_t)3 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0x10000001;
+    QTest::newRow("SRA") << ALU_OP_SRA \
+                         << (std::uint32_t)0 \
+                         << (std::uint32_t)0x80000008 \
+                         << (std::uint8_t)3 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0xF0000001;
+    QTest::newRow("SLLV") << ALU_OP_SLLV \
+                         << (std::uint32_t)3 \
+                         << (std::uint32_t)0x80000001 \
+                         << (std::uint8_t)0 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0x8;
+    QTest::newRow("SRLV") << ALU_OP_SRLV \
+                         << (std::uint32_t)3 \
+                         << (std::uint32_t)0x80000008 \
+                         << (std::uint8_t)0 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0x10000001;
+    QTest::newRow("SRAV") << ALU_OP_SRAV \
+                         << (std::uint32_t)3 \
+                         << (std::uint32_t)0x80000008 \
+                         << (std::uint8_t)0 \
+                         << Registers() \
+                         << Registers() \
+                         << (std::uint32_t)0xF0000001;
+    // JR and JALR should have no effect and we test that in core (it really doesn't make sense to test it here)
     QTest::newRow("MOVZ") << ALU_OP_MOVZ \
                          << (std::uint32_t)22 \
                          << (std::uint32_t)0 \
diff --git a/qtmips_machine/tests/testcore.cpp b/qtmips_machine/tests/testcore.cpp
index 1e5b502..c38eb26 100644
--- a/qtmips_machine/tests/testcore.cpp
+++ b/qtmips_machine/tests/testcore.cpp
@@ -82,7 +82,7 @@ static void core_regs_data() {
     regs_init.write_gp(24, 0x800000f0);
     regs_init.write_gp(25, 3);
     Registers regs_res(regs_init);
-    regs_res.write_gp(26, 0x8000001e);
+    regs_res.write_gp(26, 0xF000001e);
     QTest::newRow("SRA") << Instruction(0, 0, 24, 26, 3, 3) \
                          << regs_init \
                          << regs_res;