Skip to content

[WIP] Cleanup and refactoring + fixes #2

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

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

[WIP] Cleanup and refactoring + fixes #2

wants to merge 10 commits into from

Conversation

wargio
Copy link
Member

@wargio wargio commented Mar 27, 2025

No description provided.

@wargio wargio changed the title Cleanup and refactoring + fixes [WIP] Cleanup and refactoring + fixes Mar 27, 2025
@moste00
Copy link
Collaborator

moste00 commented Apr 1, 2025

Thanks for the contributions!

Would it be ok if I cherry-pick individual commits and apply them alone in a seperate branch?

I say this because some of the changes need some discussions and maybe alternatives. Something like flattening enum definitions is straightforward and uncontraversial, although I think we might as well just flatten all data types if we're already flattening enums. For consistency.

But I kinda disagree about replacing the input list-of-files with many input files named with release numbers? wouldn't this mean that we can only generate code per each sail-riscv release ? I don't know how often is that, but I prefer not to be constrained and be able to generate for any commit hash. A seperate commit hash file is also useful for when the CI is fixed (the CI uses the hash file to know which version of sail-riscv to fetch from github, it must do that because it asserts on identical generated code to committe generated code. None of that works right now, but it hopefully will.) Also, I think we need to think of a way to automatically generate the list of files by instrumenting the CMake script, I have seen some ways of doing that but I have been postponing it because it's fiddly, requires modifying the CMake file in a non-automatable-way.

Also, I don't think there is a reason the generator should generate spc and opt_spc itself? There is the RISCVAst2StrHelpers.h for all the opaque functions the generator can't handle (what the generator calls Intrinsic_logic, which I agree is a terrible name), they are implemented as macros so it's just as efficient as if their bodies are generated directly.

Anyway, just wanted to say I really appreciate the gesture and I'm asking if I could "plagiarize" some of your commits in a seperate branch :'D

Cheers

@moste00
Copy link
Collaborator

moste00 commented Apr 1, 2025

I forgot to mention that if your objection to the hash file was the fact it's a seperate file, then I plan to address that very soon by unifiying all configuration in a single .json (or maybe .yaml) file, along with all the things I wrote in the constants.ml file, which is also configuration.

@wargio
Copy link
Member Author

wargio commented Apr 2, 2025

nono, i just wanted to uniform stuff.

the issue i have is with files like hash.txt when it can be just an argument, meanwhile for the sail files, i prefer if we use releases so we know it's usable.

if your plan is to write a script which generates a json or similar with all the info, then i'm ok with this, but i would still suggest to be by release using the hash as value in the headers.

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