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

Add CSR encodings for Go and remove call to Go fmt #329

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

markdryan
Copy link
Contributor

This PR updates go_utils.py so that it generates a map of CSR numbers to CSR names. It also removes the call to go fmt in go_utils.py which doesn't work and always generates an error.

An example of what the new inst.go file looks like and how it will be used can be found in the links below

@markdryan
Copy link
Contributor Author

cc @4a6f656c and @mengzhuo

go_utils.py Outdated
return nil
}

var CSRs = map[uint16]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest naming this csrs so that we're not exporting it by default. We can then provide a function or variable that does export it, if we choose to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable is exported as we'll need to access the CSR map from another package (cmd/asm/internal/arch) so that we can add the CSR names to the riscv64SpecialOperand map.

See https://go-review.googlesource.com/c/go/+/630519/2/src/cmd/asm/internal/arch/riscv64.go

Given this is the case, do you still prefer that we export it from another file in the riscv64 package and not from inst.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the patch so that the generated map is not exported. In the WIP Go patch that consumes the new inst.go, I export the map from cpu.go

@4a6f656c
Copy link
Contributor

It also removes the call to go fmt in go_utils.py which doesn't work and always generates an error.

FWIW this used to work... the reason this now errors is due to the file containing an internal import (cmd/internal/obj) - this is now prohibited by newer versions of Go when the file is not also within cmd/internal.

go_utils.py Outdated
@@ -49,13 +55,11 @@ def make_go(instr_dict: InstrDict):
instr_str += f""" case A{i.upper().replace("_","")}:
return &inst{{ {hex(opcode)}, {hex(funct3)}, {hex(rs1)}, {hex(rs2)}, {signed(csr,12)}, {hex(funct7)} }}
"""
for num, name in csrs:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the ordering here (would probably be a good idea to ensure that it is sorted)?

Copy link
Contributor Author

@markdryan markdryan Dec 18, 2024

Choose a reason for hiding this comment

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

The entries in the source code are ordered by CSR number, rather than CSR name the order that the CSRs appear in csrs.csv. See

https://go-review.googlesource.com/c/go/+/630518/2/src/cmd/internal/obj/riscv/inst.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'll double check that the order is guaranteed.

Copy link
Contributor Author

@markdryan markdryan Dec 18, 2024

Choose a reason for hiding this comment

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

The ordering of the CSRs in our Go map is determined by the ordering of the CSRs in csrs.csv. Note this is neither numerical order nor are the CSRs ordered by their string names, so good point. I agree it would be better to explicitly sort them. I'll update the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the patch to sort the CSRs by CSR number. The resulting inst.go file can be seen here

Copy link
Contributor

@4a6f656c 4a6f656c left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@4a6f656c
Copy link
Contributor

4a6f656c commented Jan 8, 2025

/cc @aswaterman

@aswaterman
Copy link
Member

Thanks for the ping.

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (383cbca) to head (069b22e).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   96.53%   96.56%   +0.03%     
==========================================
  Files          10       10              
  Lines         750      757       +7     
==========================================
+ Hits          724      731       +7     
  Misses         26       26              

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

@aswaterman
Copy link
Member

@markdryan can you fix the CI failure and ping me afterwards?

  /home/runner/work/riscv-opcodes/riscv-opcodes/go_utils.py:3:8 - error: Import "subprocess" is not accessed (reportUnusedImport)

go_utils.py currently runs go fmt to pretty print the generated
inst.go file. Unfortunately, this call to go fmt always fails as
inst.go contains an internal import and go fmt is being run out of
tree. Here we remove the code that attempts to format inst.go
resulting in a clean run of go_utils.py without any errors. The
generated file, inst.go, can be formatted when it's copied into the
Go source tree, prior to its submission to the Golang project.
go_utils.py now generates a Go map that maps CSR numbers to CSR
names. The resulting map is written into inst.go for use by the
Go assembler.
@markdryan
Copy link
Contributor Author

@aswaterman the CI failure should be fixed now.

@aswaterman
Copy link
Member

Thanks.

@aswaterman aswaterman merged commit 3c73392 into riscv:master Jan 9, 2025
10 checks passed
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.

3 participants