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

Add MIPS ISA #290

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Add MIPS ISA #290

merged 3 commits into from
Sep 28, 2023

Conversation

SteliosKaragiorgis
Copy link
Contributor

No description provided.

@mortbopet
Copy link
Owner

As with any open-source contribution, i'd recommend that you:

  1. clearly specify what this change is
  2. break out your PR into separate, atomic PRs (making it possible to review - the size of this PR is not reviewable).
  3. Adhere to the style guidelines of the repo (clang-format, don't have commented out code in your PR)
  4. make sure that any dependencies have been taken care of (i assume this change depends on Active path for MIPS Processors VSRTL#57?)

@SteliosKaragiorgis
Copy link
Contributor Author

To be honest I am not so familiar with git such as split the PRs into atomic PRs. We did this project on a different file so after 1 year we needed to folk this project, copy the files into this and change some lines so it will be compatible with qt6

@mortbopet
Copy link
Owner

To be honest I am not so familiar with git such as split the PRs into atomic PRs

No worries, I can try to guide you through this as much as possible. We're in no rush - you should see this as a learning opportunity as well. any open source contribution (or closed source, if you're in a workplace) will require you to do the above to ensure code quality and that your peers understand what you've done. And you, as a programmer, will benefit in having other people review and question your on your design descisions such that you can improve the work at hand and your own skillset.

@SteliosKaragiorgis
Copy link
Contributor Author

I searched some thing on how can I split the PRs and I found out that they use terminal commands for linux. Can I do it on Windows or do I need to use linux terminal?

@mortbopet
Copy link
Owner

My best advice would be to first familiarize yourself with the process of contributing to open source projects (e.g. https://opensource.guide/how-to-contribute/, https://docs.github.com/en/get-started/quickstart/contributing-to-projects ...) and how to use git and github - google is your friend here.

Next up is deciding how you want to submit this large codechange, again you can find information about this online. When having to do the same i personally ask myself what the set of atomic changes and features are in the change that i try to make, write those out, and then submit each of those changes as a separate PR. Practically speaking, you'd do this by creating a new branch and then applying the file edits of the code changes that you decided on. This can then be submitted as a PR.

@SteliosKaragiorgis
Copy link
Contributor Author

Okay I will try

@SteliosKaragiorgis
Copy link
Contributor Author

SteliosKaragiorgis commented Aug 14, 2023

I created a new branch on my repository. Can I upload the changes here or do I need to create new pull request?

@SteliosKaragiorgis
Copy link
Contributor Author

Also does my code need to pass all the github tests?

@mortbopet
Copy link
Owner

I created a new branch on my repository. Can I upload the changes here or do I need to create new pull request?

You would need to create pull requests against this repository, just as you've done with the PR we're discussing in now.

Also does my code need to pass all the github tests?

Yes, you cannot introduce code which breaks Ripes, that wouldn't be so good for the hundreds of other people who rely on it 😉.

@SteliosKaragiorgis
Copy link
Contributor Author

So I need to create new branch, then to pull request my changes to master branch. Then the code must past all the github check. Next I need to pull request the changes here and you will check the changes
Right?

@SteliosKaragiorgis
Copy link
Contributor Author

Now I removed all the changes so I can push one by one

@SteliosKaragiorgis
Copy link
Contributor Author

I have ready the PRs to upload them but some checks failed and I dont know how to find the problem. On qt everything is working fine and with VSRTL too on mortbopet/VSRTL#59

@SteliosKaragiorgis
Copy link
Contributor Author

As I mentioned on mortbopet/VSRTL#59 I have ready small, atomic, PRs which do not depend on the changes in VSRTL. The only problem now is the tests because the tests are for RISCV ISA. Is it okay to upload the PRs to check them without these test for MIPS ISA?

@mortbopet
Copy link
Owner

ready small, atomic, PRs

Good! Sounds like you have multiple PRs ready. Another suggestion is - don't try to do too much work up front before you start submitting these PRs. The whole point of the review process is that, first, you submit a small atomic change, and that change will be reviewed. The review process may highlight things which you need to change - and those changes may then break the PRs that you have planned after that.

So, to avoid trying to boil the ocean here, i suggest that you submit one PR at a time, then we get that reviewed and checked in, and during that review process we'll determine whether or not a test should accompany that change.

@SteliosKaragiorgis
Copy link
Contributor Author

I have a lot of small PRs because some PRs are needed for the program to work correctly

Add MIPS32 to isainfo.h
@SteliosKaragiorgis
Copy link
Contributor Author

I submitted one PR to check it and tell me if you want it like this. If it is okay I will try to upload some othe small PRs to check them. Sorry for the mess with the PRs but I am still learning how to use GitHub

@mortbopet
Copy link
Owner

If it is okay I will try to upload some othe small PRs to check them. Sorry for the mess with the PRs but I am still learning how to use GitHub

No worries - are you referring to #308?

@SteliosKaragiorgis
Copy link
Contributor Author

Yes

@mortbopet
Copy link
Owner

great - i think a fine cut point for a single PR that pertains to the ISA is to provide the whole ISA definition, e.g. the equivalent of:

  1. https://github.com/mortbopet/Ripes/blob/master/src/isa/rvisainfo_common.h
  2. https://github.com/mortbopet/Ripes/blob/master/src/isa/rv32isainfo.h / https://github.com/mortbopet/Ripes/blob/master/src/isa/rv64isainfo.h

Add registers, syscall numbers and instructions opcodes and functions(for R-TYPE)
@SteliosKaragiorgis
Copy link
Contributor Author

Like this?

@mortbopet
Copy link
Owner

Like this?

Yep!

src/isa/mips32isainfo.h Show resolved Hide resolved
src/isa/mips32isainfo.h Outdated Show resolved Hide resolved
src/isa/mipsisainfo_common.h Outdated Show resolved Hide resolved
src/isa/mipsisainfo_common.h Outdated Show resolved Hide resolved
src/isa/mipsisainfo_common.h Outdated Show resolved Hide resolved
src/isa/mipsisainfo_common.h Outdated Show resolved Hide resolved
src/isa/mipsisainfo_common.h Show resolved Hide resolved
src/isa/mips32isainfo.h Outdated Show resolved Hide resolved
@SteliosKaragiorgis
Copy link
Contributor Author

So now do I need to upload new commits with the things you told me to change or I can resolve it without new commits?

@mortbopet
Copy link
Owner

So now do I need to upload new commits with the things you told me to change or I can resolve it without new commits?

You should:

  1. address the comments by making the necessary changes in your code
  2. Commit the changes to the branch that this PR was made from (https://github.com/SteliosKaragiorgis/Ripes-with-MIPS)
  3. Once you push the changes, you will see that this PR updates automatically.

Also see:

@SteliosKaragiorgis
Copy link
Contributor Author

I resolved some of your suggestions. For some others that I am not sure I commented below from your suggestions

@SteliosKaragiorgis
Copy link
Contributor Author

I think now is okay. Check it and let me know

@mortbopet mortbopet merged commit c56dfb2 into mortbopet:master Sep 28, 2023
6 checks passed
@mortbopet
Copy link
Owner

Looks good to me - merged!

@SteliosKaragiorgis
Copy link
Contributor Author

Perfect! So what is the next step?

@SteliosKaragiorgis
Copy link
Contributor Author

I mean which files do I need to upload

@mortbopet
Copy link
Owner

Well, now you just continue the process that you did with this PR - i.e. look at your code and identify the next step/piece of code that is a candidate for an atomic PR, just like you did with this PR.
I think it would be smart of you to spend some time and plan out how you approach to split up the code that you have now and figure out in which order that you want to submit PRs for it.

If you're asking in practical terms, your PR is now merged into the master branch (c56dfb2) which means that you should rebase your existing work on top of the master branch - read:

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.

2 participants