fix(gen): support subtype-based encodings in C header generator #904

blazethunderstorm · 2025-07-15T23:12:06Z

Fixes #893

Updated the generator.py to correctly detect instruction encodings when using the new type/subtype schema. Previously, it looked only for the old encoding field, so some instructions were skipped. Now it checks subtype fields and extracts encoding info from there.

Fixes riscv-software-src#893

ThinkOpenly

There are WAY too many unrelated changes to easily review this. Could you create a single commit with just the necessary changes to address the issue?

Fixes riscv-software-src#893

blazethunderstorm · 2025-07-15T23:19:27Z

@ThinkOpenly pls see now

ThinkOpenly · 2025-07-16T02:00:02Z

I don't think these changes have any impact. Are you seeing some problems being resolved?

I would expect that instead of finding the needed "match" string directly, you'd need to compute it by going into the "format" attribute and its children "opcodes" and "variables".

Note that these changes would likely appear in backends/generators/generator.py in load_instructions(). Indeed when you run ./do gen:c_header, you get error messages where these issues occur:

ERROR:: Missing 'encoding' field in instruction add.uw in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/Zba/add.uw.yaml
ERROR:: Missing 'encoding' field in instruction rolw in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/rolw.yaml
ERROR:: Missing 'encoding' field in instruction rol in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/rol.yaml
ERROR:: Missing 'encoding' field in instruction xnor in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/xnor.yaml
ERROR:: Missing 'encoding' field in instruction clmul in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/clmul.yaml
ERROR:: Missing 'encoding' field in instruction orn in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/orn.yaml
ERROR:: Missing 'encoding' field in instruction clmulh in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/clmulh.yaml
ERROR:: Missing 'encoding' field in instruction andn in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/andn.yaml
ERROR:: Missing 'encoding' field in instruction rorw in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/rorw.yaml
ERROR:: Missing 'encoding' field in instruction ror in /workspace/riscv-unified-db/gen/resolved_spec/_/inst/B/ror.yaml

Part of your mission is to make those error messages go away. The current code just extracts the value of the "match" attribute, but you'll need to compute it.

blazethunderstorm · 2025-07-16T13:27:42Z

@ThinkOpenly would do that

dhower-qc · 2025-07-16T15:00:31Z

Thanks for the contribution! Like @ThinkOpenly said, you'll need to build up what used to be explicit in 'match'. Basically, start with an instruction-length string of '-'s and then replace any position that is occupied by an opcode value.

So if you have:

# andn.yaml
format:
  funct7:
    display_name: ANDN
    location: 31-25
    value: 0b0100000
  funct3:
    display_name: ANDN
    location: 14-12
    value: 0b111
  opcode:
    display_name: OP
    location: 6-0
    value: 0b0110011

You want to wind up with the string:

0100000----------111-----0110011

blazethunderstorm · 2025-07-16T15:08:52Z

@dhower-qc thanks for help would make the changes as req

ThinkOpenly

Thanks again for your efforts. Good code. Comments/questions inline.

backends/generators/generator.py

ThinkOpenly · 2025-07-16T16:17:35Z

backends/generators/generator.py

+                    encoding_filtered += 1
+                    continue
+
+                continue 


does this have any effect?

@blazethunderstorm can you see this review? It also seems randomly placed to me

The continue statement is still there, but looking again it does have a purpose: the code above handles the "format" case, and the code below handles the "encoding" case. These are mutually exclusive, so OK.

Nit: could you remove the extra blank line above the continue statement?

still there :-)

backends/generators/generator.py

AFOliveira · 2025-07-23T11:42:35Z

@blazethunderstorm I believe @ThinkOpenly comments were not addressed.

AFOliveira

@blazethunderstorm not to this one, I thin k

AFOliveira · 2025-07-23T13:54:26Z

backends/generators/generator.py

+                    encoding_filtered += 1
+                    continue
+
+                continue 


@blazethunderstorm can you see this review? It also seems randomly placed to me

AFOliveira · 2025-07-25T09:45:23Z

@blazethunderstorm You still have not addressed @ThinkOpenly comments, can you please take a look at them?

blazethunderstorm · 2025-08-02T21:34:29Z

@ThinkOpenly @AFOliveira pls review

AFOliveira

Please finish that small loose end. I plan to re-test Golang generation and C Header this week to see if we are finaly close enough, and getting this PR in before I try it, would be great!

AFOliveira · 2025-08-05T07:51:04Z

@AFOliveira sry to ask but what loose end like I have to fix

@ThinkOpenly comment.

codecov · 2025-08-07T15:48:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.05%. Comparing base (292b34f) to head (c76ff94).
⚠️ Report is 46 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #904   +/-   ##
=======================================
  Coverage   46.05%   46.05%           
=======================================
  Files          11       11           
  Lines        4942     4942           
  Branches     1345     1345           
=======================================
  Hits         2276     2276           
  Misses       2666     2666

Flag	Coverage Δ
idlc	`46.05% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ThinkOpenly · 2025-08-08T22:23:44Z

CI is failing, complaining about the extra blank line and three other formatting issues.

AFOliveira

Looks good to me, please fix the commit history!

ThinkOpenly · 2025-08-11T14:36:08Z

backends/generators/generator.py

+                    encoding_filtered += 1
+                    continue
+
+                match_bits = ["-" for _ in range(32)]


Does this work for compressed (extension "C" and friends) instructions? For example:

riscv-unified-db/spec/std/isa/inst/C/c.addi.yaml

Line 21 in 292b34f

match: 000-----------01

fixed pls review

ThinkOpenly · 2025-08-11T14:43:44Z

backends/generators/generator.py

+                                )
+                                val_bin = bin(val_int)[2:].zfill(width)
+
+                                match_bits[31 - hi : 32 - lo + 1] = list(val_bin)


Also needs to accommodate compressed instructions (16 bit encoding)

ThinkOpenly

I don't think the recent changes work in practice.

To determine the actual length of the instruction encoding you probably need to:

For instructions which have been converted to "format" style
1. Access the instruction type's "length" attribute:
  1. Get the instruction's subtype.
  2. Get the subtype's type.
  3. Get the type's length.
2. Or, compute the instruction's length by walking through all of the "opcodes" and "variables" and accounting for all of the bits.
For instructions which have not been converted to "format" and still use "encoding", get the length of the "match" field.

ThinkOpenly · 2025-08-12T15:49:01Z

backends/generators/generator.py


-                match_bits = ["-" for _ in range(32)]
+                bit_length = 32
+                if "length" in data:


Is this ever true?

ThinkOpenly · 2025-08-12T15:50:26Z

backends/generators/generator.py

+                        pass
+                elif "C" in enabled_extensions:
+                    if any(
+                        loc.get("location", "").startswith("0..15")


Is this ever true?

ThinkOpenly · 2025-08-12T15:53:05Z

backends/generators/generator.py

                                val_bin = bin(val_int)[2:].zfill(width)

-                                match_bits[31 - hi : 32 - lo + 1] = list(val_bin)
+                                inst_width = 16 if encoding.get("compressed", False) else 32


Is encoding.get("compressed"... ever true?

ThinkOpenly · 2025-08-12T19:03:52Z

Looks good to me, please fix the commit history!

What do you seek, @AFOliveira , given that the merge queue will squash everything?

AFOliveira · 2025-08-12T19:06:42Z

Looks good to me, please fix the commit history!

What do you seek, @AFOliveira , given that the merge queue will squash everything?

Nevermind, I'm just not used to the merge queue squash.

ThinkOpenly · 2025-09-17T19:03:52Z

Superceded by #1051

fix(gen): support subtype-based encodings in generator

6ee1689

Fixes riscv-software-src#893

blazethunderstorm requested review from dhower-qc and ThinkOpenly as code owners July 15, 2025 23:12

blazethunderstorm changed the title ~~fix(gen): support subtype-based encodings in generator~~ fix(gen): support subtype-based encodings in C header generator Jul 15, 2025

ThinkOpenly requested changes Jul 15, 2025

View reviewed changes

fix(gen): support subtype-based encodings in C header generator

a57ffa7

Fixes riscv-software-src#893

blazethunderstorm requested a review from ThinkOpenly July 15, 2025 23:19

blazethunderstorm and others added 2 commits July 16, 2025 20:57

fixed the error

15172fd

Merge branch 'main' into fix-gen-subtype-encoding-893

a228ba6

ThinkOpenly requested changes Jul 16, 2025

View reviewed changes

AFOliveira requested changes Jul 20, 2025

View reviewed changes

backends/generators/generator.py Outdated Show resolved Hide resolved

made req changes

1f2f24e

blazethunderstorm requested review from AFOliveira and ThinkOpenly July 23, 2025 10:42

Merge branch 'main' into fix-gen-subtype-encoding-893

cf0cc1b

AFOliveira reviewed Jul 23, 2025

View reviewed changes

ThinkOpenly mentioned this pull request Jul 24, 2025

Missing encoding fields in B extension instructions causing code generation failures #924

Open

blazethunderstorm and others added 2 commits August 3, 2025 03:03

fixed

6a8a830

Merge branch 'main' into fix-gen-subtype-encoding-893

de7dd50

blazethunderstorm requested a review from AFOliveira August 2, 2025 21:34

AFOliveira reviewed Aug 5, 2025

View reviewed changes

blazethunderstorm and others added 2 commits August 7, 2025 20:34

fixed

af45d42

Merge branch 'main' into fix-gen-subtype-encoding-893

73d33da

blazethunderstorm requested a review from AFOliveira August 7, 2025 15:05

blazethunderstorm and others added 3 commits August 10, 2025 17:52

fixed linting issue

0388837

Merge branch 'main' into fix-gen-subtype-encoding-893

7da09ba

fixed linting issue

7d8ce3d

AFOliveira approved these changes Aug 11, 2025

View reviewed changes

ThinkOpenly reviewed Aug 11, 2025

View reviewed changes

blazethunderstorm and others added 2 commits August 12, 2025 01:23

fixed

f2e8556

Merge branch 'main' into fix-gen-subtype-encoding-893

c76ff94

blazethunderstorm requested review from AFOliveira and ThinkOpenly August 11, 2025 19:53

This comment was marked as off-topic.

Sign in to view

ThinkOpenly requested changes Aug 12, 2025

View reviewed changes

jordancarlin mentioned this pull request Aug 22, 2025

Generated C header missing instructions with new subtype schema #893

Open

AFOliveira closed this Sep 17, 2025

blazethunderstorm deleted the fix-gen-subtype-encoding-893 branch September 17, 2025 19:06

fix(gen): support subtype-based encodings in C header generator #904

fix(gen): support subtype-based encodings in C header generator #904

Uh oh!

Conversation

blazethunderstorm commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThinkOpenly left a comment

Choose a reason for hiding this comment

Uh oh!

blazethunderstorm commented Jul 15, 2025

Uh oh!

ThinkOpenly commented Jul 16, 2025

Uh oh!

blazethunderstorm commented Jul 16, 2025

Uh oh!

dhower-qc commented Jul 16, 2025

Uh oh!

blazethunderstorm commented Jul 16, 2025

Uh oh!

ThinkOpenly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AFOliveira commented Jul 23, 2025

Uh oh!

AFOliveira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AFOliveira commented Jul 25, 2025

Uh oh!

blazethunderstorm commented Aug 2, 2025

Uh oh!

AFOliveira left a comment

Choose a reason for hiding this comment

Uh oh!

AFOliveira commented Aug 5, 2025

Uh oh!

codecov bot commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ThinkOpenly commented Aug 8, 2025

Uh oh!

AFOliveira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

ThinkOpenly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThinkOpenly commented Aug 12, 2025

Uh oh!

AFOliveira commented Aug 12, 2025

Uh oh!

blazethunderstorm commented Jul 15, 2025 •

edited

Loading

codecov bot commented Aug 7, 2025 •

edited

Loading