search

mortbopet / VSRTL

Visual Simulation of Register Transfer Logic
MIT License
88 stars 18 forks source link

Active path and FSM #59

Open SteliosKaragiorgis opened 1 year ago

SteliosKaragiorgis commented 1 year ago

I reuploaded it. I removed the comments and it is qt6. Also it is working fine with the Ripes

SteliosKaragiorgis commented 1 year ago

So is it better to upload the PRs on Ripes https://github.com/mortbopet/Ripes/pull/290 without my VSRTL changes and then add them?

mortbopet commented 1 year ago

Before considering PRs into either repository you need to address the above comments. This will require that you sit down, look at your current implementation, identify where you're adding things into VSRTL that shouldn't be in VSRTL (as described above) and then figure out a way to reimplement that either:

  1. outside of VSRTL (i.e. in your processor model implementation (the stuff that uses VSRTL which you're going to eventually PR into Ripes)
  2. If not possible outside of VSRTL (i.e. in your implementation), this means that you need to consider a change to VSRTL itself.

I cannot stress this enough. Whereever you have non-generic code inside VSRTL this is an indication that you need to change that code. VSRTL is a framework and thus implementation-specific logic should not be anywhere within the framework.

And the same goes with an eventual Ripes PR. If you study the current Ripes codebase you will see that ISA-specific information is only found in two places:

  1. The processor models: https://github.com/mortbopet/Ripes/tree/master/src/processors/RISC-V
  2. The ISA information files: https://github.com/mortbopet/Ripes/tree/master/src/isa

So similarly, wherever your code does not follow this pattern means that you'll have to refactor it.

SteliosKaragiorgis commented 1 year ago

Okay. First I will try to reimplement it outside the VSRTL. I think I tried it again on the past but I will try again. The problem is that I added some lines that cointains some things from Ripes, correct? Like if(QString::fromStdString(m_component->getHierName()).contains("MIPS"))

SteliosKaragiorgis commented 1 year ago

Do you want first to upload my PRs on Ripes without the VSRTL changes?

mortbopet commented 1 year ago

The problem is that I added some lines that cointains some things from Ripes, correct? Like if(QString::fromStdString(m_component->getHierName()).contains("MIPS"))

Correct!

Do you want first to upload my PRs on Ripes without the VSRTL changes?

If you have small, atomic, PRs which do not depend on the changes in VSRTL, and represent incremental improvements to Ripes, then by all means, go ahead and submit those PRs! :)

SteliosKaragiorgis commented 1 year ago

Yes I have small PRs ready to upload but I have some tests that fail because it is for different ISA. I mentioned exact the prblem here: https://github.com/mortbopet/Ripes/pull/290