Add initial Golang generation support. by AFOliveira · Pull Request #526 · riscv/riscv-unified-db

Afonso Oliveira (AFOliveira) · 2025-03-21T16:05:46Z

This PR aims to generate inst.go - currently checked out for reviewing purposes. It doesn't yet allow for subsets of the ISA, but serves as an example that it is much easier to generate the outputs than the riscv-opcodes.

Also, I think I found some CSRs that we are missing, I'll open a issue on that.

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) · 2025-03-22T11:19:14Z

Mark Ryan (@markdryan) I'll tag you like this since I can't ask for review in any other way. This is still only "All" the RV64 ISA, I'll make compartimentalized on Monday

Mark Ryan (markdryan)

I've had a quick look. Note my comments are based on the version of inst.go generated by the first commit. It doesn't look like you re-generated inst.go when pushing the second commit, so the comments might not all be relevant.

Afonso Oliveira (AFOliveira) · 2025-03-24T10:00:46Z

I've had a quick look. Note my comments are based on the version of inst.go generated by the first commit. It doesn't look like you re-generated inst.go when pushing the second commit, so the comments might not all be relevant.

They are, I did not change much other than filtering yet, thanks!

…ne set of extensions that GO currently supports. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

…generator.py may be used to generate. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Paul Clarke (ThinkOpenly) · 2025-03-25T21:50:52Z

I don't have any particular concerns after quickly looking at the code. Is the generated artifact for inst.go match the target?

For Spike, same question. I don't think there's a relationship between Golang and Spike... should this be a separate PR?

Afonso Oliveira (AFOliveira)

I think my answers did not show because I did not comment here, can you see them Mark Ryan (@markdryan)?

Afonso Oliveira (AFOliveira) · 2025-03-26T14:58:50Z

I don't have any particular concerns after quickly looking at the code. Is the generated artifact for inst.go match the target?

For Spike, same question. I don't think there's a relationship between Golang and Spike... should this be a separate PR?

So, for Golang I think we are there or, at least super close (some order may be changed, but that is it). Same for Spike, but I agree, I will remove Spike out of the branch since it just adds confusion, wait for this to be merged and then get Spike on board as well.

Derek Hower (dhower-qc)

Can you add a tasks.rake in backends/generators/Go with a task that will be displayed with ./do --tasks?

Afonso Oliveira (AFOliveira) · 2025-03-26T15:15:08Z

Can you add a tasks.rake in backends/generators/Go with a task that will be displayed with ./do --tasks?

I already did locally actually :) Let me just remove Spike, and solve the commit history and I will change this PR from draft to ready to review. Unfortunately, it seems CSRs information in the UDB is the weak-point ATM, although I think we will change this in a matter of weeks

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Derek Hower (dhower-qc) · 2025-03-26T16:46:33Z

Derek Hower (@dhower-qc) @kbroch-rivosinc Do you guys have any clue on why the tests are failing? https://github.com/riscv-software-src/riscv-unified-db/actions/runs/14088608154/job/39459215925?pr=526#step:6:142

It looks like you are stuck in the middle here. GitHub thinks it needs to check for C# files, which is true in main. But your branch doesn't have any C# files, which is why I think it's failing. Hopefully after you merge with main the error will resolve.

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) · 2025-03-26T16:52:41Z

I think this is ready to merge - only problems are:

Inherently some instructions order is different than riscv-opcodes generated inst.go, I don't think there is a way to solve this. (also, tiny scope, I think it only happens once in the whole inst.go)
We are missing some essential CSRs, so we can not officially propose this to the Go community, I propose just relying on an issue to track this and merge this PR into main as is. Whenever, we have the same CSRs as RISC-V Opcodes I'll just update everyone.

Paul Clarke (ThinkOpenly) · 2025-03-26T16:56:21Z

Inherently some instructions order is different than riscv-opcodes generated inst.go, I don't think there is a way to solve this. (also, tiny scope, I think it only happens once in the whole inst.go)

Could you post a diff here, so we have a reference?

We are missing some essential CSRs, so we can not officially propose this to the Go community, I propose just relying on an issue to track this and merge this PR into main as is. Whenever, we have the same CSRs as RISC-V Opcodes I'll just update every one.

OK

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) · 2025-03-26T20:04:54Z

Could you post a diff here, so we have a reference?

https://www.diffchecker.com/srkhmI0D/
The left one is riscv-opcodes and the right is UDB generated.

What I was mentioning as order change happens on line 921.
There are other differences in instructions but this is a question of how riscv-opcodes describes data vs how UDB does, it may require discussions but has nothing to do with this PR.
You may see the CSR diff in the end.

…all. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Derek Hower (dhower-qc) · 2025-03-26T20:17:28Z

Cool. The instructions look pretty good.

We have pretty good issue coverage for the CSRs.

I notice that the riscv-opcodes version isn't correct for pmpcfgODD; if I understand correctly, this is supposed to be for RV64, and those CSRs don't exist in RV64.

I'd vote to approve this PR since the generation looks great. We can work on the missing data.

Afonso Oliveira (AFOliveira) · 2025-03-26T20:22:31Z

Cool. The instructions look pretty good.

We have pretty good issue coverage for the CSRs.

I notice that the riscv-opcodes version isn't correct for pmpcfgODD; if I understand correctly, this is supposed to be for RV64, and those CSRs don't exist in RV64.

I'd vote to approve this PR since the generation looks great. We can work on the missing data.

Nice, let me just add this to the regression test then, just to ensure it is not broken along the way

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

+    runs-on: ubuntu-latest
+    env:
+      SINGULARITY: 1
+    steps:
+      - name: Clone Github Repo Action
+        uses: actions/checkout@v4
+      - name: Setup apptainer
+        uses: eWaterCycle/setup-apptainer@v2.0.0
+      - name: Get container from cache
+        id: cache-sif
+        uses: actions/cache@v4
+        with:
+          path: .singularity/image.sif
+          key: ${{ hashFiles('container.def', 'bin/.container-tag') }}
+      - name: Get gems and node files from cache
+        id: cache-bundle-npm
+        uses: actions/cache@v4
+        with:
+          path: |
+            .home/.gems
+            node_modules
+          key: ${{ hashFiles('Gemfile.lock') }}-${{ hashFiles('package-lock.json') }}
+      - if: ${{ steps.cache-sif.outputs.cache-hit != 'true' }}
+        name: Build container
+        run: ./bin/build_container
+      - name: Generate Go code
+        run: ./do gen:go


Afonso Oliveira (AFOliveira) · 2025-03-27T15:50:44Z

Is there any dependency for this PR Derek Hower (@dhower-qc)? Or can we merge this and then I'll start the rest of riscv-opcodes outputs?

Derek Hower (dhower-qc)

Looks good to me. We might want to eventually elevate the python interface into the db data to lib, but let's do that later

* Add initial Golang generation support. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Enhance go generator to allow filtering by extensions * Enhance Go CSR generation by filtering out unused extensions and define set of extensions that GO currently supports. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * update checked out version of inst.go changed by latest commits. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Refactor to ensure reusable code, given the extensibility of outputs generator.py may be used to generate. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Fix Go awkward struct spacing. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Add rake task to generate Golang input - inst.go Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Add CSR ordering to match riscv-opcodes. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Remove checked out inst.go Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Update all spacing to exactly match riscv-opcodes. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Change check_requirement due to being out of scope for go_generator call. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Add gen:go to CI regression. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> * Add gen:go to local regression test. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> --------- Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com> Co-authored-by: Your Name <your.email@example.com>

Add initial Golang generation support.

4a3025a

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) requested a review from Paul Clarke (ThinkOpenly) March 22, 2025 11:17

Enhance go generator to allow filtering by extensions

1fc7590

Mark Ryan (markdryan) reviewed Mar 24, 2025

View reviewed changes

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Afonso Oliveira added 3 commits March 24, 2025 11:19

Enhance Go CSR generation by filtering out unused extensions and defi…

5b3a379

…ne set of extensions that GO currently supports. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

update checked out version of inst.go changed by latest commits.

60bdf6c

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Refactor to ensure reusable code, given the extensibility of outputs …

a0e4abc

…generator.py may be used to generate. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) commented Mar 26, 2025

View reviewed changes

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Comment thread backends/generators/Go/inst.go Outdated

Derek Hower (dhower-qc) suggested changes Mar 26, 2025

View reviewed changes

Afonso Oliveira added 2 commits March 26, 2025 16:32

Fix Go awkward struct spacing.

a2fe524

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Add rake task to generate Golang input - inst.go

f92f410

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) force-pushed the AFOliveira/GoSupport branch from 891783b to f92f410 Compare March 26, 2025 16:33

Afonso Oliveira added 2 commits March 26, 2025 16:47

Add CSR ordering to match riscv-opcodes.

362b01e

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Remove checked out inst.go

89873fa

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Afonso Oliveira (AFOliveira) marked this pull request as ready for review March 26, 2025 16:50

Merge branch 'main' into AFOliveira/GoSupport

7d3c6f7

Update all spacing to exactly match riscv-opcodes.

11461ed

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Change check_requirement due to being out of scope for go_generator c…

0505d06

…all. Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Add gen:go to CI regression.

11ec002

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

Add gen:go to local regression test.

096df87

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>

github-advanced-security AI found potential problems Mar 26, 2025

View reviewed changes

Derek Hower (dhower-qc) approved these changes Mar 27, 2025

View reviewed changes

Derek Hower (dhower-qc) mentioned this pull request Mar 27, 2025

RISC-V Opcodes Data Cross-Validation #449

Closed

Merge branch 'main' into AFOliveira/GoSupport

45b59c4

Afonso Oliveira (AFOliveira) removed the request for review from Paul Clarke (ThinkOpenly) March 28, 2025 10:08

Afonso Oliveira (AFOliveira) added this pull request to the merge queue Mar 28, 2025

Merged via the queue into main with commit 3b854dc Mar 28, 2025

Afonso Oliveira (AFOliveira) deleted the AFOliveira/GoSupport branch March 28, 2025 10:17

Sahil Kumar (Samyra312007) mentioned this pull request Aug 3, 2026

parse_extension_requirements never matches UDB definedBy format, so --extensions filtering in Go/C-header/SystemVerilog generators is broken #2344

Open

Uh oh!

Conversation

Afonso Oliveira (AFOliveira) commented Mar 21, 2025

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 22, 2025

Uh oh!

Mark Ryan (markdryan) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 24, 2025

Uh oh!

Paul Clarke (ThinkOpenly) commented Mar 25, 2025

Uh oh!

Afonso Oliveira (AFOliveira) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 26, 2025

Uh oh!

Derek Hower (dhower-qc) left a comment

Choose a reason for hiding this comment

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 26, 2025

Uh oh!

Derek Hower (dhower-qc) commented Mar 26, 2025

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Paul Clarke (ThinkOpenly) commented Mar 26, 2025

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 26, 2025

Uh oh!

Derek Hower (dhower-qc) commented Mar 26, 2025

Uh oh!

Afonso Oliveira (AFOliveira) commented Mar 26, 2025

Uh oh!

Check warning

Afonso Oliveira (AFOliveira) commented Mar 27, 2025

Uh oh!

Derek Hower (dhower-qc) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Afonso Oliveira (AFOliveira) commented Mar 26, 2025 •

edited

Loading