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.

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

MTCannon: Increase differential test coverage across opcodes #12032

Closed
mbaxter opened this issue Sep 20, 2024 · 15 comments · Fixed by #12808
Closed

MTCannon: Increase differential test coverage across opcodes #12032

mbaxter opened this issue Sep 20, 2024 · 15 comments · Fixed by #12808
Assignees

Comments

@mbaxter
Copy link
Contributor

mbaxter commented Sep 20, 2024

Description

Increase differential test coverage across MIPS opcodes. We have some basic coverage with the existing open mips tests, but we should try to get more coverage around edge cases. Look into adding more tests and porting existing tests from MIPS.t.sol.

New tests should follow the single-step unit test pattern as in this example, rather than adding more assembly tests. The list of supported instructions is available in the README. Documentation on MIPS instructions can be found here.

@mbaxter mbaxter added the A-cannon Area: cannon label Sep 20, 2024
@GrapeBaBa
Copy link
Contributor

@mbaxter I see a series of issues related to cannon testing. Are these issues already in development, and are they suitable for external contributors like us to contribute?

@mbaxter
Copy link
Contributor Author

mbaxter commented Sep 23, 2024

@GrapeBaBa - no one is assigned to this yet, so happy to have external contributors take a look! Let me know if you have any questions.

@Inphi
Copy link
Contributor

Inphi commented Sep 23, 2024

@GrapeBaBa would you be able to take on this issue?

@GrapeBaBa
Copy link
Contributor

@GrapeBaBa would you be able to take on this issue?

Yes

@Inphi
Copy link
Contributor

Inphi commented Sep 27, 2024

Looks like this is done. Thanks!

@Inphi Inphi closed this as completed Sep 27, 2024
@GrapeBaBa
Copy link
Contributor

Looks like this is done. Thanks!

@Inphi I think it is still a lot of tests need to port, I plan to use this task tracking all the sub tasks, can you reopen it?

@Inphi
Copy link
Contributor

Inphi commented Sep 27, 2024

Ah right. Sorry about that.

@mbaxter
Copy link
Contributor Author

mbaxter commented Oct 3, 2024

@GrapeBaBa - just noticed that the new LoadStore tests fell through the cracks wrt the 64-bit refactor. Would you mind updating these to use the new Word architecture-specific variables?

@GrapeBaBa
Copy link
Contributor

@GrapeBaBa - just noticed that the new LoadStore tests fell through the cracks wrt the 64-bit refactor. Would you mind updating these to use the new Word architecture-specific variables?

Ok, will do it.

@Inphi
Copy link
Contributor

Inphi commented Oct 23, 2024

@GrapeBaBa Are we good to close out this ticket?

@GrapeBaBa
Copy link
Contributor

There are still a few tests that need to be migrated. If it's not necessary, I suggest not closing it for now.

@Inphi
Copy link
Contributor

Inphi commented Oct 24, 2024

Gotcha. Let's keep this issue open then. The remaining instruction tests to be migrated are:

  • - jalr
  • - lui
  • - srav
  • - sll
  • - srl
  • - clz
  • - clo
  • - srlv
  • - bgtz
  • - bne
  • - beq
  • - jump

Feel free to create sub-issues for these as needed.

@BlocksOnAChain
Copy link

@GrapeBaBa - hey, wanted to check about your plans to continue work on this issue, since it would be great to close the remaining work?

@GrapeBaBa
Copy link
Contributor

should be finished this weekend

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

Successfully merging a pull request may close this issue.

4 participants