Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Instruction Test #120

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Add Instruction Test #120

wants to merge 9 commits into from

Conversation

trdthg
Copy link
Contributor

@trdthg trdthg commented Apr 18, 2024 •
edited

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
jdupak reviewed Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@jdupak jdupak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/machine/instruction.test.cpp Outdated
// 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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for this constructor

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

indeed seems useless, I'll delete it.

src/machine/instruction.cpp Outdated
@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

src/machine/instruction.cpp Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great.

src/machine/instruction.cpp Outdated
#include <iostream>
#include <fstream>
#include <qfile.h>
void gen() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also move it to the test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@jdupak
Copy link
Collaborator

jdupak commented Apr 18, 2024

I will address the other questions shortly.

@jdupak
Copy link
Collaborator

jdupak commented Apr 18, 2024

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
Copy link
Collaborator

jdupak commented Apr 18, 2024

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
Copy link
Collaborator

jdupak commented Apr 18, 2024

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

@trdthg trdthg changed the title add test Add Instruction Test Apr 25, 2024
@trdthg trdthg marked this pull request as ready for review April 25, 2024 18:42
@jdupak jdupak marked this pull request as draft May 2, 2024 09:29
@jdupak
Copy link
Collaborator

jdupak commented May 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdupak jdupak jdupak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@trdthg @jdupak