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

proposal: cmd/compile: add support for RISC-V B-extension #47373

Closed
benshi001 opened this issue Jul 24, 2021 · 9 comments
Closed

proposal: cmd/compile: add support for RISC-V B-extension #47373

benshi001 opened this issue Jul 24, 2021 · 9 comments
Assignees
Projects
Milestone

Comments

@benshi001
Copy link
Member

@benshi001 benshi001 commented Jul 24, 2021

The specification of bitmanip instructions became stable from draft: https://github.com/riscv/riscv-bitmanip

So I propose to support these instructions, which could make following benefits.

  1. Accelerate several math.bits functions, such as bits.CountLeadingZeros
  2. Accelerate logal and/ori/xori, for example, "x=x&0xfffeffff" can be simplifed to a single instruction "bclri $16, Reg"
  3. integer extension can be simplifed with a single "bext" from current pari of arithmetic left/right shift.
  4. others

However, there is no real world hardware support them now, but we can implement them in the assmbler by now.

One concern, Go's riscv64 implies the i-a-m-f-d extensions (also known as rv64g ), do we need something like GORISCV64=GB to enable the B-extension instrutions?

@benshi001 benshi001 self-assigned this Jul 24, 2021
@randall77
Copy link
Contributor

@randall77 randall77 commented Jul 24, 2021

Supporting assembly seems fine.

One concern, Go's riscv64 implies the i-a-m-f-d extensions (also known as rv64g ), do we need something like GORISCV64=GB to enable the B-extension instrutions?

Where are the minimum extensions for riscv64 specified? I don't see an entry in https://github.com/golang/go/wiki/MinimumRequirements . Could you add an entry?

Yes, we can't use B extension instructions in the compiler without introducing a GORISCV64 environment variable or guarding them at runtime.
For 1, it's probably worth it to use them with a runtime guard. We do that for popcount on amd64, for example.
The other two probably aren't better enough than their non-B equivalents to warrant a runtime guard. And I don't think they are in total better enough to warrant a GORISCV64 modifier. But that's a guess, actual benchmarks would be helpful once hardware exists.

Loading

@ALTree ALTree changed the title [proposal][riscv] Support the B-extension instructions proposal: Support the riscv B-extension instructions Jul 24, 2021
@gopherbot gopherbot added this to the Proposal milestone Jul 24, 2021
@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Jul 25, 2021

Supporting assembly seems fine.

One concern, Go's riscv64 implies the i-a-m-f-d extensions (also known as rv64g ), do we need something like GORISCV64=GB to enable the B-extension instrutions?

Where are the minimum extensions for riscv64 specified? I don't see an entry in https://github.com/golang/go/wiki/MinimumRequirements . Could you add an entry?

I have added, that the minimal requirement is rv64g (rv64imafd).

Yes, we can't use B extension instructions in the compiler without introducing a GORISCV64 environment variable or guarding them at runtime.
For 1, it's probably worth it to use them with a runtime guard. We do that for popcount on amd64, for example.
The other two probably aren't better enough than their non-B equivalents to warrant a runtime guard. And I don't think they are in total better enough to warrant a GORISCV64 modifier. But that's a guess, actual benchmarks would be helpful once hardware exists.

I suggest using a GORISCV64 environment, and gcc/clang use the command line option -march to indicate instruction set.

Loading

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Jul 25, 2021

One more reason for support a GORISCV64 env-var, is that there might be rv64 machines without a HW floating point unit. All FP calculation must be implemented via software functions.

For example, we reqiure the minimal should be I-M-A, and F-D-B-V-P are optional. So the GORISCV64 is a combination of F-D-B-V-P with a prefix IMA.

Loading

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Jul 28, 2021

I think we should detect extension in runtime cpu feature detection.

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 28, 2021
@rsc rsc moved this from Incoming to Active in Proposals Aug 4, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 4, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@rsc rsc changed the title proposal: Support the riscv B-extension instructions proposal: cmd/compile: add support for RISC-V B-extension Aug 4, 2021
@4a6f656c
Copy link
Contributor

@4a6f656c 4a6f656c commented Aug 5, 2021

While there is no significant downside to adding support for these instructions in the assembler, I also think there is benefit in waiting until there is actual hardware support for them. Furthermore, we should be confirming that the use of these instructions is actually providing benefit, which means we need to be able to benchmark.

IMO as far as possible we should avoid compile time options and perform runtime detection - I've seen various problems with compile time options (like GOARM and GO386), particularly with binary packaging. While you may be building on a machine that supports one feature set, you end up having to opt for the lowest common denominator as the packages may be installed on a machine that does not support it.

Lastly, the current Go riscv64 port currently targets rv64g (which is effectively a baseline for most real world RISC-V hardware) - I would be very reluctant to remove F and D and require those to be explicitly turned back on (largely for the same reasons as mentioned above) - additionally removing these from the minimal target is going to create additional maintenance and require additional testing for the port.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

It sounds like we should decline this issue until there is actual hardware that we can use to measure the impact of B.

There was a suggestion above to lower the current minimum feature set, removing F and D. That is a separate discussion, and if you feel strongly about that, I would suggest filing a separate proposal.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Decline in Proposals Aug 11, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Aug 18, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants