Add CSR encodings for Go and remove call to Go fmt #329
Conversation
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
Outdated
| return nil | ||
| } | ||
|
|
||
| var CSRs = map[uint16]string { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
FWIW this used to work... the reason this now errors is due to the file containing an internal import ( |
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: | |||
There was a problem hiding this comment.
What's the ordering here (would probably be a good idea to ensure that it is sorted)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
But I'll double check that the order is guaranteed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've updated the patch to sort the CSRs by CSR number. The resulting inst.go file can be seen here
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
force-pushed
the
markdryan/go-csr
branch
from
6fcd9f3 to
a1e90ca
Compare
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