Convert torture tests to unit tests

[pavelkryukov]

Blocked by #589

To test functionality of instruction, we use SPIM torture tests (TT). That is not very convinient, as we have to build Binutils and traces in addition to unit tests. The idea is to convert the TT test cases into unit test cases.

Example:

    li $2, 1
    li $4, 0
    sllv $3, $2, $4
    bne $3, 1, fail

becomes

TEST_CASE("sllv")
{
    MIPSInstr<MIPS32> instr( "sllv"); // You need #589 to do that
    instr.set_v_src1( 1);
    instr.set_v_src2( 0);
    instr.execute();
    CHECK( instr.get_v_dst(), 1);
}

There are about 120 instructions supported by MIPT-MIPS so far. Let's assume that adding tests for 17 instructions is a 1 point, so adding tests to 119 instructions is 7 points. You are not obliged to get all the 7 points at once, you may make 1 point and stop, then continue later or pass the task to another project participant.

[pavelkryukov]

Just to clarify: torture tests are located here: https://github.com/MIPT-ILab/mips-traces/blob/master/tt.core.universal.s

[pavelkryukov]

@pooh64 What's the progress with that?

[pukhovv]

@pavelkryukov Now I'm learning mips assembler, I haven't written new tests yet.

[pukhovv]

Implemented, but not tested instructions:

1. beql
2. bgez
3. bgezal
4. bgezall
5. bgezl
6. bgtz
7. bgtzl
8. blez
9. blezl
10. bltz
11. bltzal
12. bltzall
13. bltzl
14. bnel

[bova-ev]

@pavelkryukov Can I write to the HI and LO registers that are used as destination registers in instruction madd (for example)? As I can see I can change only one destination register by using instr._set_vdst() function.

[pavelkryukov]

instr.set_v_dst() sets destination value, not the register name. You do not need that method as you have destination value generated by execute(). In fact, madd is a tricky instruction, as it does multiplication on ALU and summation in RF, so basically you have to test it with the same test set as mult, checking that accumulation flag (I don't recall the name at the moment, but you can quickly look it up in RF code) is 1.

[pavelkryukov]

For loads and stores, you may create a FuncMemory instance:

TEST_CASE( /* */ )
{
    auto func_memory = FuncMemory::create_plain_memory( 10); // Creates a 10 bit address memory (1K)
// ...
    func_memory->load( instr); // Loads value to register
}

[pavelkryukov]

@DavidRabbitson Do you intend to proceed?

[bova-ev]

@pavelkryukov I've done pretty much work on MIPT subjects during this long peroid of time. Unfortunately, I haven't done much on MIPS simulator (my apologies) but I've learnt a lot of new theoretical stuff about logics and uArchitechture apart from lectures. Now I have a free week from my studies due to my illness so I hope I'll proceed on this task.

[pavelkryukov]

Please report that you are not going to finish task due to external circumstances as early as you are aware of it. I expected that to be completed in months, so we can proceed with implementation of RISC-V and big-endian MIPS. If I'd knew there was no much progress with that, I'd complete it myself.

[bova-ev]

Ok, got that. Hope this situation won't happen anymore. Can I send my first pack of tests for some instructions this evening?

[pavelkryukov]

Sure, thanks.

[bova-ev]

@pavelkryukov I have mess with commits. Can I delete my forked repository and do all commits in a correct way?

[pavelkryukov]

Do not worry, I usually squash all commits to a new commit. You may open a pull request as is.

[bova-ev]

@pavelkryukov Why do instructions msub and msubu pass all mult tests? If I understand correctly, results should be multiplied by (-1) because for example msub rs, rt algorithm looks like this: (HI, LO) <- (HI, LO) - (GRP[rs] x GPR[rt]).

[pavelkryukov]

Subtraction (and addition in case of madd) happens inside register file, both in HW and our simulation.

If generic ALU did that operation, it would read 4 registers: HI, LO, rs and rt. Instead, MIPS architects put an additional double-width ALU inside RF over the HI/LO registers. The motivation for these instructions was to simplify operations like matrix multiplication, where each cell is calculated as a1 * b1 + a2 * b2 + .... However, it complicates hardware (for data bypass, you still have to track 4 registers), and RISC-V ISA developers do not intent to use similar instructions.

[bova-ev]

@pavelkryukov DADD seems to be 64 bit instruction due to its description, but it is checked as MIPS32Instr in disassembly test block. https://github.com/MIPT-ILab/mipt-mips/blob/76c7fdc5c7e75a78b22d3a363d0ee217cf3581a6/simulator/mips/t/unit_test.cpp#L91 Why? Should I test it as MIPS64Instr?

[pavelkryukov]

Why?

It does not make any harm that MIPS64 instruction is disassembled even in MIPS32 mode; but if such instruction is executed, it would raise a trap.

Should I test it as MIPS64Instr?

I think yes.

[bova-ev]

@pavelkryukov Should I put trap check in every first test case of 64 bit instruction or I can do it like here? https://github.com/MIPT-ILab/mipt-mips/blob/7f76610f1e38620027a650b75ced2609c7364502/simulator/mips/t/unit_test.cpp#L52-L57

[pavelkryukov]

Should I put trap check in every first test case of 64 bit instruction or I can do it like here?

I suggest to wrap that check to a function:

static bool not_a_mips32_instruction( std::string_view name)
{
    MIPS32Instr instr( name);
    instr.execute(); 
    return instr.trap_type() == Trap::UNKNOWN_INSTRUCTION;
} 

to have it in a single line:

TEST_CASE("dcloz")
{
    CHECK( not_a_mips32_instruction("dcloz") );
// ...
}

[pavelkryukov]

Also, please move all MIPS64 tests to a separate translation unit — the current file is already huge.

[bova-ev]

You mean create new unit test file? How should I call it to avoid conflicts with compilation?

[pavelkryukov]

I think mips32_test.cpp and mips64_test.cpp should be fine.

[bova-ev]

CHECK( not_a_mips32_instruction( "dadd")) = false CHECK( not_a_mips32_instruction( "daddu")) = false Looks like dadd and daddu are 32 bit instructions according to these checks. Is that ok?

[pavelkryukov]

Oops. that's a bug. Our decoder says they were available in MIPS I (but MIPS III actually). Could you please fix it? https://github.com/MIPT-ILab/mipt-mips/blob/7f76610f1e38620027a650b75ced2609c7364502/simulator/mips/mips_instr.cpp#L218-L219

[bova-ev]

I'll try.

[bova-ev]

Do I need to test Load_Store instructions in little endian and big endian both like lw? https://github.com/MIPT-ILab/mipt-mips/blob/3f403a1c5a5185d3d83be64da646208a2e5c5018/simulator/mips/t/unit_test.cpp#L1061 https://github.com/MIPT-ILab/mipt-mips/blob/3f403a1c5a5185d3d83be64da646208a2e5c5018/simulator/mips/t/unit_test.cpp#L1076

[pavelkryukov]

Let's start with little-endian. I consider big-endian tests as a separate issue (#413).

[bova-ev]

Ok. Is that ok for you that I ask so many questions? I can do it less frequently if you want me to.

[pavelkryukov]

That's far from "many".

[bova-ev]

Do I have to check exceptions in load_store instructions unit tests? How do I do that?

[pavelkryukov]

If instruction entered a trap, it has instr.trap_type() set to some value, so you need to do something like:

CHECK( instr.trap_type() == Trap::UNALIGNED_ADDRESS);

[bova-ev]

Also I see that every load_store instruction is running at both 32 bit and 64 bit systems. Do I need to test them in 64 bit as well?

[pavelkryukov]

There is some discrepancy between the behavior in 32 bit and 64 bit mode. I do not remember what is the point, but I suspecte it is related to sign extension. Could you please tell what is different in 32 bit and 64 bit tests?

[bova-ev]

I'm not sure if this is significant difference but in 32 bit MIPS sign extended value we get after lb (0xab should be loaded) is 0xffff'ffab and in 64 bit it is 0xffff'ffff'ffff'ffab just because GPR [rt] is 64 bit long.

[pavelkryukov]

Ok, I agree that difference may be ignored in unit tests. The issue was that MIPS code has to use different immediates.

[pavelkryukov]

Leaving 2 points as 2 points were committed to the score table.

[bova-ev]

@pavelkryukov ll instruction provides primitives to implement atomic Read-Modify-Write (RMW) operations for cached memory locations. How can I test this RMW part of this instruction? Or I can do ll tests similar to lh tests?

[pavelkryukov]

We do not have a data cache model so far, therefore it should behave like a simple load of the similar size.

[bova-ev]

ldr and ldl instructions returns the same result with same arguments. I think problem is located here: https://github.com/MIPT-ILab/mipt-mips/blob/8a2335b7cc91c059fca891039aff12ffacf40169/simulator/mips/mips_instr.cpp#L76-L77 Do I need to fix this or this is ok?

[pavelkryukov]

If you are able to write manual on these instructions (#413), then you can implement them. But at the moment, we should just skip them, since they are not implemented.

[bova-ev]

How can I read memory created by this function? https://github.com/MIPT-ILab/mipt-mips/blob/8bcc2a3c0a22a3dcd291c687de675ce8c9ba8493/simulator/mips/t/unit_test.cpp#L1065-L1070

[pavelkryukov]

auto m = get_plain_memory_with_data();
auto value = m->read<uint32, Endian::little>( address);

[bova-ev]

Can I write templates in unit_test.cpp?

[pavelkryukov]

Yes, but could you please share your intention? I don't feel there is a good idea...

[bova-ev]

@pavelkryukov Why do I get dst = 0xdeadbeef when I test bgezal -1? Shouldn't I get dst = PC + 8?

[pavelkryukov]

Looks like a bug — $ra should be updated regardless of branch being taken or not taken. Could you please fix it?

[bova-ev]

Does likely branches do the same thing in simulator as basic branches?

[pavelkryukov]

Likely branches are not supported at the moment, so you may write tests but probably they won't pass. The general idea is that likely branches do not execute delay slot instruction.