search

mortbopet / Ripes

A graphical processor simulator and assembly editor for the RISC-V ISA
https://ripes.me/
MIT License
2.57k stars 274 forks source link

Add MIPS ISA #290

Closed SteliosKaragiorgis closed 1 year ago

mortbopet commented 1 year ago

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 https://github.com/mortbopet/VSRTL/pull/57?)
SteliosKaragiorgis commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Okay I will try

SteliosKaragiorgis commented 1 year ago

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

SteliosKaragiorgis commented 1 year ago

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

mortbopet commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

SteliosKaragiorgis commented 1 year ago

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 https://github.com/mortbopet/VSRTL/pull/59

SteliosKaragiorgis commented 1 year ago

As I mentioned on https://github.com/mortbopet/VSRTL/pull/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 commented 1 year ago

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 commented 1 year ago

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

SteliosKaragiorgis commented 1 year ago

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 commented 1 year ago

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 https://github.com/mortbopet/Ripes/pull/308?

SteliosKaragiorgis commented 1 year ago

Yes

mortbopet commented 1 year ago

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
SteliosKaragiorgis commented 1 year ago

Like this?

mortbopet commented 1 year ago

Like this?

Yep!

SteliosKaragiorgis commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

SteliosKaragiorgis commented 1 year ago

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

mortbopet commented 1 year ago

Looks good to me - merged!

SteliosKaragiorgis commented 1 year ago

Perfect! So what is the next step?

SteliosKaragiorgis commented 1 year ago

I mean which files do I need to upload

mortbopet commented 1 year ago

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 (https://github.com/mortbopet/Ripes/commit/c56dfb2b97a5b8ca61a77536b671d8b6f1c289ee) which means that you should rebase your existing work on top of the master branch - read: