Closed SteliosKaragiorgis closed 8 months ago
I dont know why it uploads the previous PRs
To avoid me repeating myself too much (😉) remember to go through all of the suggestions and comments that i placed on your previous PR, and also apply them here. The first part of any review process should be self review (e.g. you taking a critical look at the diff and thinking about whether a change is warranted.
Just as an example, a brief skim of the current diff tells me that there's MIPS constructs in the assembler, which is (just like we've discussed for VSRTL) a no go.
I made these changes because I used them on other file. Do i need to upload all the files or one by one?
So is it wrong that i used SignedMips
?
I uploaded mips relocations
Is it okay to upload the other PRs?
You should probably follow through with this PR before uploading the rest.
As the PR stands right now, i think it's a bit too small - you want small, atomic commits, yes; but ideally you also want something to use that PR. Right now, nothing is using this file so the things aren't even being built! Meaning that i (the reviewer) have no idea whether you're submitting code that actually compiles.
In this way i have to upload all the files of assembler because they are all used together
I hope it will be okay now
PR'ing the entire assembler at once seems like an OK granularity for the contribution; as a final thing, you should also include some assembler tests so we know that it's actually working.
Secondly, we just had a refactor of how instructions are represented in the assembler, s.t. they are no longer macro based but instead class based, so the PR needs a refactor to adhere to that (see https://github.com/mortbopet/Ripes/commit/27fcc58c02b26e84156ef588d42fb36d8ada4543).
... and maybe also watch out/wait for https://github.com/mortbopet/Ripes/pull/314 to land, since that will require another change!
I never worked with tests, so I dont know how to implement a test for the assembler
I never worked with tests, so I dont know how to implement a test for the assembler
Then i would suggest you to look at the existing RISC-V tests (the ones i linked) which should give you a good idea about what assembler testing entails.
Okay then. For the assembler do I need to change it completely and do it like https://github.com/mortbopet/Ripes/commit/27fcc58c02b26e84156ef588d42fb36d8ada4543 ?
Do I need to do a new assembler test for MIPS or just edit this test ?
Do I need to do a new assembler test for MIPS or just edit this test ?
If you look at the folder hierarchy, you'll notice these tests are nested within risc-v
-specific folders. As I've mentioned before, to have proper MIPS support in Ripes, you essentially have to do generally the same things as have been done for risc-v specific things - this also includes MIPS-specific tests.
Since we (at this point in the process) don't have any execution capability for MIPS-related stuff, i think it would suffice to add a test like the one you linked (https://github.com/mortbopet/Ripes/blob/master/test/tst_assembler.cpp) called tst_mips_assembler.cpp
to ensure basic capabilities. Once execution capability is here, you should look into creating comprehensive self-tests like the assembly programs listed here: https://github.com/mortbopet/Ripes/tree/master/test/riscv-tests. Note that all of those self-tests have been grabbed from somewhere else - there's plenty of RISC-V ISA test suites out there, I'm sure there's also some for MIPS.
How can I connect the tst_mips_assembler.cpp
with MIPS assembler?
@SteliosKaragiorgis as I've mentioned a few times before, the basic exercise here is to 1) look at what is done for RISC-V in Ripes and 2) do essentially the same thing for MIPS, but in a decoupled way such that the code is maintainable and of high quality. This implies that you have to spend time researching the codebase, and trying different things - I can't give exact instructions to every change.
For instance, a quick glance at tst_assembler should give you an idea about how you can call the MIPS assembler.
This PR is to upload the MIPS Assembler. For now I updated the instruction.h