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

Adding SLTUI instruction #710

Closed
wants to merge 1 commit into from
Closed

Adding SLTUI instruction #710

wants to merge 1 commit into from

Conversation

galloj
Copy link
Contributor

@galloj galloj commented Oct 16, 2022

Instruction is documented here: https://edumips64.readthedocs.io/en/latest/instructions.html but is not implemented. If I am right it should be same as SLTIU, so I basically copied how it is done in DADDUI.java.

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Merging #710 (38f4799) into master (d92aea6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #710      +/-   ##
============================================
- Coverage     51.14%   51.13%   -0.02%     
  Complexity     1447     1447              
============================================
  Files           249      250       +1     
  Lines          9879     9882       +3     
  Branches       1092     1092              
============================================
  Hits           5053     5053              
- Misses         4507     4510       +3     
  Partials        319      319              
Impacted Files Coverage Δ
...java/org/edumips64/core/is/InstructionBuilder.java 83.03% <0.00%> (-0.75%) ⬇️
src/main/java/org/edumips64/core/is/SLTUI.java 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d92aea6...38f4799. Read the comment docs.

@lupino3
Copy link
Member

lupino3 commented Oct 18, 2022

Hey @galloj, thanks a lot for your contribution!

I believe that what you found is not a missing instruction, but an error in the documentation!

I cannot find an SLTUI instruction in the MIPS64 instruction set documentation: https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MIPS_Architecture_MIPS64_InstructionSet_%20AFP_P_MD00087_06.05.pdf

However, there is an SLTIU instruction, and that is what is both implemented in code (see SLTIU.java and described in the docs you pointed at.

Since I'd like to preserve a history of your contribution, would you be willing to fix the documentation, replacing the word SLTUI with the word SLTIU? If you'd prefer, I can also fix it.

Thanks again for finding and reporting this problem!

@galloj
Copy link
Contributor Author

galloj commented Oct 18, 2022

I wasn't really sure if it was missing instruction or error in documentation, but I chose to believe it is missing instruction because both versions of instruction can be found here in your repository: https://github.com/EduMIPS64/edumips64/blob/2b5c97e7df5c0b2a21f7d16f60eba889b5e944b1/contrib/kate_mips.xml

Also when googling for SLTUI instruction, then there are some results (although about 10 times less).

Anyways, it is likely that my guess might be wrong, so if you prefer the fix to documentation, then I will do it.

@lupino3
Copy link
Member

lupino3 commented Oct 18, 2022

Yes please! As a reference for the instructions et we follow the MIPS64 official docs I linked earlier, and there is no mention of the SLTUI instruction there.

Thank you very much!

@galloj
Copy link
Contributor Author

galloj commented Oct 18, 2022

Okay, new PR is opened.

@galloj galloj closed this Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants