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

A better version of the "Why a new Design" #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PythonLinks
Copy link

Thank you for the excellent feedback. Usually when I approach a new project, I try to update its documentation to make it friendlier to newbies like me. Often the pull requests get ignored without explanation. In your case you gave a clear explanation, and so I am able to update my pull request. I just deleted the old version, forked again and am issuing a new pull request.

It was very helpful to update the documentation. I now understand that the newer version is easier to understand, that is what I should be starting with, particularly since my goal is getting educated, rather than a specific client deliverable.

There are now two versions of "Why a New Design?" One for new users, one for existing users. I am not sure what you will want to do about that.

I also corrected some of the English in your original version. I am a native English speaker. I also speak French.

Thank you for the excellent feedback.  Usually when I approach a new project, I try to update its documentation to make it friendlier  to newbies like me.  Often the pull requests get ignored without explanation.  In your case you gave a clear explanation, and so I am able to update my pull request.  I just deleted the old version, forked again and am issuing a new pull request. 

It was very helpful to update the documentation.  I now understand that the newer version is easier to understand, that is what I should be starting with, particularly since my goal is getting educated, rather than a specific client deliverable. 

There are now two versions of "Why a New Design?"  One for new users, one for existing users.  I am not sure what you will want to do about that. 

I also corrected some of the English in your original version.  I am a native English speaker.  I also speak French.
Copy link
Member

@Dolu1990 Dolu1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

I have hardtime with documentation, that is nice that people reading it point out / pr what isn't good :D

There are now two versions of "Why a New Design?" One for new users, one for existing users. I am not sure what you will want to do about that.

I'm thinking that maybe the "why a new design" could integrated into the main "About VexiiRiscv" ? I mean the "About VexiiRIscv" is mostly about telling all the upside and is the first thing people willl read.

@@ -52,17 +52,31 @@ Here is a list of important assumptions and things to know about :
- In the execute pipeline, stage.up(RS1/RS2) is the value to be used, while stage.down(RS1/RS2) should not be used, as it implement the bypassing for the next stage
- Fetch.ctrl(0) isn't persistant.

About VexRiscv (not VexiiRiscv)
Why a New Design?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a New Design

Hmm maybe "new design" is too much implicit (maybe it should be more explicit)
"Why VexiiRiscv" ?

------------------------------------

There is few reasons why VexiiRiscv exists instead of doing incremental upgrade on VexRiscv
The original VexRiscv is a successful design. But we have reached the limits of what it can accomplish, and in the process of improving it, we added too much complexity. We now have a newer and better abstraction. Specifically the new VexiiRiscv abstraction:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"better abstraction" can make people running away. Also, fondamentaly it isn't about the abstraction, it is more about the framework :)

- Has a much cleaner frontend / branch prediction design.
- Has a more flexible plugin system.
- Has a much better verification approach.
- Works better with DDRAM than the existing write through data cache.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this is aproximative. The VexRiscv design works quite well with SDR DRAM. The notion of running the CPU at higher frequency with write through was starting to hit DRAM limits is kinda key.
(also DRAM not DDRAM right ? )


Really almost the whole VexRiscv system would need to rewritten, so it is better to start anew. This can be done faster than carrying around the old baggage.

What was Wrong with the Old Design?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Old Design" is too much relying on the implicit understanding that we talk about VexRiscv.
So maybe it should refer to VexRiscv directly


There are a few reasons why we are creating a new VexiiRiscv instead of doing incremental upgrade on VexRiscv:

- Almost all of the VexRiscv parts need to be upgraded.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost all of the VexRiscv parts need to be upgraded.

It isn't that they need to be ugraded, it is more that something better could be done.

This first commit responds to your recommended changes.
As you suggested. 

it is very common that active developers have less energy for the documentation.  Particularly if it is in a foreign language.  
On the other hand to attract new users, it is critical.  The saying is:  "You only get one chance to make a  good first impression. "

And from a newbies perspective reading the documentation is so much easier than trying to read the source code.  Like a friend of mine said:  "Poor documentation is a show stopper. "

You have done the harder work of creating all of this over the last 5 years.  So I am happy to help some with the documentation.  Thank you for your patience when I get it wrong.
- Providing a cleaner implementation, getting ride of the technical debt, especially the frontend
- Proper branch prediction
- ...
VexiiRiscv is the next generation of VexRiscv, enabling things which are not possible with the current VexRiscv framework. The new VexiiRiscv framework:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next generation

The wording isn't great it think, because it is time sensitive. While "second iteration" is timeless.

Also overall it lose the meaning that VexiiRiscv is a from scratch implementation

framework

I would say this should be in the bullet point, not as an introduction text

- ...
VexiiRiscv is the next generation of VexRiscv, enabling things which are not possible with the current VexRiscv framework. The new VexiiRiscv framework:

- Supports both the 32 bit and the 64 bit Risc-V Instruction sets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We losed the definition of what extentions are supported IMAFDC

Hmm overall, i didn't meant to replace the first paragram / bullet points with a direct VexRiscv comparison, but instead to add the comparison in the same chapter. Something short and direct.

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