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

Simplify go's LUT #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify go's LUT #332

wants to merge 1 commit into from

Conversation

Jorropo
Copy link

@Jorropo Jorropo commented Jan 3, 2025

I am working on C support and it started being very awkward to have to add things like funct4 and funct6 when the bits overlap and the some of the fields are used differently or not used at all by different encodings.

By passing the match the go assembler can do a simple bitwise or. This was also changed to emit go fmt-ed code from the start as go fmt now refuse to run due to the internal import.

Here is the matching CL on go's side https://go-review.googlesource.com/c/go/+/637940

I am working on C support and it started being very awkward
to have to add things like funct4 and funct6 when the bits overlap
and the some of the fields are used differently or not used at all
by different encodings.

By passing the match the go assembler can do a simple bitwise or.
This was also changed to emit go fmt-ed code from the start as go fmt
now refuse to run due to the internal import.

Here is the matching CL on go's side https://go-review.googlesource.com/c/go/+/637940
@aswaterman
Copy link
Member

Can another user of the Go target look at this and provide feedback?

@mengzhuo
Copy link
Contributor

mengzhuo commented Jan 8, 2025

CC @4a6f656c @markdryan as Go riscv64 maintainers

@Jorropo
Copy link
Author

Jorropo commented Jan 8, 2025

I would be surprising to have any user of the go target because it import private packages that are only found in the go compiler:

import "cmd/internal/obj"

@4a6f656c
Copy link
Contributor

4a6f656c commented Jan 8, 2025

Can another user of the Go target look at this and provide feedback?

I expect that the only consumer is Go itself, however this needs a lot more discussion and review upstream, before a change of this nature is made.

@Jorropo
Copy link
Author

Jorropo commented Jan 8, 2025

To copy my rational from https://go-review.googlesource.com/c/go/+/637940?tab=comments

Among multiples my biggest issue is funct3, I am adding support for CI, CSS, CIW, CL, CS, CB and CJ which use bits 13 to 15, however this script always generate funct3 as bits 12 to 14 because this is what R encoding uses. Passing rv_c to this script will generate the C instructions but funct3 is incorrect (bits 12 to 14).

So at this point I started writing a primitive disassembler in this script to figure out where funct3 must be decoded from.
But then I asked myself « why am I doing this ? »
This script takes the fixed bits of the instructions and convert it into constituent parts.
Go then takes all the constituent parts and shift them in place to rebuild the fixed bits.
The overall process is no-op.

The existing code also does not follow go's programing conventions, returning a pointer here is uncommon and eat performance uselessly.

Given this whole process is confusing and serves no purpose I think it is better to remove it rather than to fix it for CI, CSS, CIW, CL, CS, CB and CJ.
If we do not remove this we need to make it even more confusing by adding funct4 and funct6 which means nothing for most instructions.

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.

4 participants