Add Instruction Test #120

trdthg · 2024-04-18T18:45:20Z

Related to #7

This demo currently adds some test cases for instruction_to_str(), and also includes a demo function to generate test cases

Most of the test cases are generated automatically, one for each instruction (except for the first 7, which I first tried to write by hand, but it was too inefficient).
Registers, Imm, addresses are simply set to 1 (or other values as needed)

@jdupak

Do you have any suggestions on the method I'm using here, or provide more test topics?
What about programloader tests, should we now simply hand write tests to cover as many instructions and section types as possible, or look for ways to reuse the sail-riscv test set? (The latter seems pretty complicated.)

jdupak

Amazing work so far. I only have some organizational comments.

jdupak · 2024-04-18T18:54:47Z

src/machine/instruction.test.cpp

-    //    QCOMPARE(Instruction(0x4432146), Instruction(1, 2, 3, 4, 5, 6));
-    //    QCOMPARE(Instruction(0x4430004), Instruction(1, 2, 3, 4));
-    //    QCOMPARE(Instruction(0x4000002), Instruction(1, 2_addr));
+    QCOMPARE(Instruction(0x0), Instruction(Instruction(0x0)));


I am confused by Instruction(Instruction(...)). What is it supposed to mean?

just for this constructor

Instruction::Instruction(const Instruction &i) { this->dt = i.data(); }

indeed seems useless, I'll delete it.

jdupak · 2024-04-18T19:09:59Z

src/machine/instruction.cpp

@@ -1514,3 +1514,133 @@ TokenizedInstruction::TokenizedInstruction(
    , address(address)
    , filename(std::move(filename))
    , line(line) {}
+
+
+void gen_instruction_code_and_expected_str(QTextStream& outfile, const InstructionMap* im, uint32_t code, int count, int offset, int subclass_i, int depth) {


This should be in the instruction.test.cpp file. It has no use during normal operation of the simulator. Also I would prefer the name "generate_code_and_string_data" and a little docstring explaining the usage.

jdupak · 2024-04-18T19:12:24Z

src/machine/instruction.cpp

+
+    QString test_code = QString::asprintf("QCOMPARE(Instruction(0x%x).to_str(), \"%s\");", code, qPrintable(res));
+    outfile << test_code;
+    if (Instruction(code).to_str() != res) {


I am not sure why this is here. I would expect the test generation and execution to be separate.

Should it be in a separate file as an executable? (for generating test cases)

Sounds great.

jdupak · 2024-04-18T19:13:20Z

src/machine/instruction.cpp

+#include <iostream>
+#include <fstream>
+#include <qfile.h>
+void gen() {


Could you make the name more descriptive? Also, there should be an empty line after the includes.

Also move it to the test file.

same as above

jdupak · 2024-04-18T19:16:34Z

I will address the other questions shortly.

jdupak · 2024-04-18T20:18:26Z

What about programloader tests, should we now simply hand write tests to cover as many instructions and section types as possible, or look for ways to reuse the sail-riscv test set? (The latter seems pretty complicated.)

I don't think we need to dig into the instructions here. We need to test the loading functionality. I think it would be nice to come up with one executable with various sections and then "randomly" check the contents of the memory.
As for sail-riscv, as far as I understand, they are just binaries of the official risc-v test, which we are running already (see the CI).

jdupak · 2024-04-18T20:31:30Z

Do you have any suggestions on the method I'm using here, or provide more test topics?

I like your approach. Since you have the pairs already generated, I would love to add also the other direction (code_from_string).

After that, there are many commented-out MIPS tests in the core.test.cpp. Converting them to RISC-V would be great.

jdupak · 2024-04-18T20:34:11Z

We will be able to close #7 once all QSKIP calls are gone.

jdupak · 2024-05-02T09:32:05Z

Good work so far. Some notes.

You will need to clean up the commits a bit.
Try making the inputs more random. x1 and immediate zero is not so great test.

trdthg · 2024-06-26T02:42:11Z

Completely refactored

Now instruction.test.gendata.cpp iterates through all the instructions on instruction_map, tries to generate 100 (code and string_data)pair for each, then checks to_str and code_from_string and fails if they don't match.

The current generation logic relies on arg_desc's min and max.

This is an executable file, the output looks like this:

...
inst: [jalr] 

  - [ 0]    code: [  fb268267] str: [             jalr x4, -78(x13)]
  - [ 1]    code: [  7dce8a67] str: [           jalr x20, 2012(x29)]
  - [ 2]    code: [  cfc78ee7] str: [           jalr x29, -772(x15)]
  ...

inst: [j] 

  - [ 0]    code: [  e32f006f] str: [                     j -0xf9ce]
  ...
...

Since it can't generate a fixed test case every time you run this script, the file name shouldn't be gendata anymore. I tried to move the code from main to instruction.test.cpp, but fails.

Because there are too many structures used here that instruction.cpp doesn't expose (e.g. ArgumentDesc, C_inst_map, fill_argdesbycode ...), So I include instruction.cpp directly.

@jdupak could you give me some advise?

jdupak reviewed Apr 18, 2024

View reviewed changes

trdthg force-pushed the issue7 branch 2 times, most recently from e77d427 to 1e6e49f Compare April 25, 2024 09:15

trdthg changed the title ~~add test~~ Add Instruction Test Apr 25, 2024

trdthg marked this pull request as ready for review April 25, 2024 18:42

jdupak marked this pull request as draft May 2, 2024 09:29

trdthg force-pushed the issue7 branch from c50c149 to 91667bb Compare May 2, 2024 09:51

trdthg force-pushed the issue7 branch from 701cc69 to 909e87e Compare June 9, 2024 14:09

fix inst_base split

7e3bd09

trdthg force-pushed the issue7 branch from 909e87e to 4b95180 Compare June 17, 2024 03:52

trdthg mentioned this pull request Jun 24, 2024

Machine: fix CSR::RegisterMapByName key type to std::string #142

Merged

trdthg force-pushed the issue7 branch 2 times, most recently from e72c604 to a248b5c Compare June 26, 2024 02:10

refactor

2777e76

trdthg force-pushed the issue7 branch from a248b5c to 2777e76 Compare June 26, 2024 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Instruction Test #120

Add Instruction Test #120

trdthg commented Apr 18, 2024 •

edited

Loading

jdupak left a comment

jdupak Apr 18, 2024

trdthg Apr 18, 2024

jdupak Apr 18, 2024

trdthg Apr 18, 2024

jdupak Apr 18, 2024

trdthg Apr 18, 2024

jdupak Apr 18, 2024

jdupak Apr 18, 2024

jdupak Apr 18, 2024

trdthg Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented May 2, 2024

trdthg commented Jun 26, 2024 •

edited

Loading

Add Instruction Test #120

Are you sure you want to change the base?

Add Instruction Test #120

Conversation

trdthg commented Apr 18, 2024 • edited Loading

jdupak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented Apr 18, 2024

jdupak commented May 2, 2024

trdthg commented Jun 26, 2024 • edited Loading

trdthg commented Apr 18, 2024 •

edited

Loading

trdthg commented Jun 26, 2024 •

edited

Loading