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 · 30 comments
Closed

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

benshi001 opened this issue Jul 24, 2021 · 30 comments
Labels
arch-riscv Issues solely affecting the riscv64 architecture. FrozenDueToAge Proposal
Milestone

Comments

@benshi001
Copy link
Member

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 added the arch-riscv Issues solely affecting the riscv64 architecture. label Jul 24, 2021
@benshi001 benshi001 self-assigned this Jul 24, 2021
@randall77
Copy link
Contributor

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.

@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 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.

@benshi001
Copy link
Member Author

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.

@mengzhuo
Copy link
Contributor

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

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

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

@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 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.

@rsc
Copy link
Contributor

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.

@rsc
Copy link
Contributor

rsc commented Aug 11, 2021

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

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

rsc commented Aug 18, 2021

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

@benshi001
Copy link
Member Author

There is realworld riscv64 hardware supporting the B extension,

https://www.sifive.com/cores/u74

and we can buy a dev-board from https://www.iceasy.com/10210/1022688923.shtml

Do we consider that

  1. support the B instructions in the assembler
  2. Write specific rules/passes in the compiler to utilize those B instructions?

@benshi001 benshi001 reopened this Feb 12, 2022
@benshi001
Copy link
Member Author

ping ...

Any further conclusion about support the B-type instruction set.

@ianlancetaylor ianlancetaylor moved this from Declined to Incoming in Proposals (old) Mar 4, 2022
@ianlancetaylor
Copy link
Contributor

Moving this back to the incoming proposal queue.

@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

The GORISCV64 variable would match all the others. The list should probably be comma-separated instead of assuming single-letter feature names. For GOAMD64 we managed to use v1, v2, v3 etc. Is there anything like that in the RISC-V world, or do we need a full feature enumeration?

And I assume that our minimum required set would be implicitly required still, no matter what GORISCV64 says?

@rsc rsc moved this from Incoming to Active in Proposals (old) Mar 9, 2022
@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

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

@benshi001
Copy link
Member Author

benshi001 commented Mar 11, 2022

The GORISCV64 variable would match all the others. The list should probably be comma-separated instead of assuming single-letter feature names. For GOAMD64 we managed to use v1, v2, v3 etc. Is there anything like that in the RISC-V world, or do we need a full feature enumeration?

And I assume that our minimum required set would be implicitly required still, no matter what GORISCV64 says?

I advocate your idea, that using comma seperated features. AFAIK, there is a g stands for the combination of i+m+a+f+d. And b/c/e are not included in the g. So we should

  1. Accept comma sperated single features, such as GORISCV64=i,m,a,f;
  2. Accept g as a combination, along with other features, such GORISV64=g,c, which means i+m+a+f+d+c;
  3. Deny conflict features, such as GORISCV64=g,e, in wchich i and e are conflict features.

@benshi001
Copy link
Member Author

There is also real HW support half floating point scala, whose name is zfh, we can also support that with GORISCV64=g,zfh.

@benshi001
Copy link
Member Author

benshi001 commented Mar 12, 2022

I did not find a real world riscv64 HW that has not the F & D extension, so I suggest the minimal to be GORISCV64=g, so all I+M+A+F+D are mandatory even the user does not specifies.

@4a6f656c
Copy link
Contributor

While I think we are closer to being able to support the B-extension, I believe a prerequisite is having a builder that supports these instructions (neither of the current builders do). It is also worth noting that the suggested hardware is likely to be have lower performance (2 cores) than the current builders (8 cores).

Also, to reiterate my previous comment, as far as possible, we should avoid compile time options and perform runtime detection instead - otherwise there are a number of cases where the lowest common denominator has to be assumed.

@4a6f656c
Copy link
Contributor

The GORISCV64 variable would match all the others. The list should probably be comma-separated instead of assuming single-letter feature names. For GOAMD64 we managed to use v1, v2, v3 etc. Is there anything like that in the RISC-V world, or do we need a full feature enumeration?

Because of the way that ISA extensions have been approached, it is theoretically possible to build a CPU that has the minimum extensions, all of the extensions or some combination of the extensions (hence full enumeration would be needed). That said, there are already groups like g which effectively expand to a set of individual extensions.

And I assume that our minimum required set would be implicitly required still, no matter what GORISCV64 says?

We would either need to add g to whatever was specified, or treat it as an error if the base extensions were not included.

@rsc
Copy link
Contributor

rsc commented Mar 16, 2022

We have a baseline of 'g' and I don't think anyone is suggesting we abandon that. So we don't need to say 'g' explicitly in the GORISCV64 list. (We could use -g if we really really needed, but it seems like essentially all hardware we want to support does g.)

This issue is just about "is it okay to support GORISCV64=b?" It seems like the answer should be yes.

For operations like bits.CountLeadingZeros which are probably a single cycle when you just emit the direct instruction, it's hard to see how any kind of runtime feature detection wouldn't be a huge performance loss.

@benshi001
Copy link
Member Author

We have a baseline of 'g' and I don't think anyone is suggesting we abandon that. So we don't need to say 'g' explicitly in the GORISCV64 list. (We could use -g if we really really needed, but it seems like essentially all hardware we want to support does g.)

This issue is just about "is it okay to support GORISCV64=b?" It seems like the answer should be yes.

For operations like bits.CountLeadingZeros which are probably a single cycle when you just emit the direct instruction, it's hard to see how any kind of runtime feature detection wouldn't be a huge performance loss.

As you suggested, if g is the implicit baseline, then GORISCV64 is not able to support e, since e is conflict with g. I think that is fine. g is much more common in real world.

@benshi001
Copy link
Member Author

While I think we are closer to being able to support the B-extension, I believe a prerequisite is having a builder that supports these instructions (neither of the current builders do). It is also worth noting that the suggested hardware is likely to be have lower performance (2 cores) than the current builders (8 cores).

The new hardware does have poor performance to become a build machine. So we can support it in the assembler first without a builder. And then we make use of B in the bits packages and SSA rules when there is a suitable build machine.

Also, to reiterate my previous comment, as far as possible, we should avoid compile time options and perform runtime detection instead - otherwise there are a number of cases where the lowest common denominator has to be assumed.

@4a6f656c
Copy link
Contributor

There is realworld riscv64 hardware supporting the B extension,

https://www.sifive.com/cores/u74

and we can buy a dev-board from https://www.iceasy.com/10210/1022688923.shtml

So apparently the Starfive VisionFive board does not support the B extensions (even though it has U74 cores) - the specification on the allnetchina website (and others) only lists rv64gc and someone with access to one advises that the FDT also has a riscv,isa value of rv64imafdc.

@4a6f656c
Copy link
Contributor

We have a baseline of 'g' and I don't think anyone is suggesting we abandon that.

It was suggested previously, but has since been retracted.

So we don't need to say 'g' explicitly in the GORISCV64 list. (We could use -g if we really really needed, but it seems like essentially all hardware we want to support does g.)

I was not suggesting it be required explicitly in GORISCV64, rather that we treat that as the default internally - any values specified via GORISCV64 would be additive to g. The other question is, lets presume that in the future we enable b by default - how do you turn it off? Is that GORISCV64=, GORISCV64=-b, GORISCV64=g or ... ?

This issue is just about "is it okay to support GORISCV64=b?" It seems like the answer should be yes.

I agree in general, however without real world hardware existing and at least one of the builders supporting this extension, I doubt we can reasonably proceed beyond assembler support.

For operations like bits.CountLeadingZeros which are probably a single cycle when you just emit the direct instruction, it's hard to see how any kind of runtime feature detection wouldn't be a huge performance loss.

Correct, there are going to be situations like this that cannot be avoided - again though, the problem is that for any situation where binary packages are being built for distribution, the lowest common denominator has to be assumed, meaning that no one benefits from these extensions in these cases.

@gopherbot
Copy link

Change https://go.dev/cl/394435 mentions this issue: cmd/internal/obj/riscv: Implement the Zbs instructions

@benshi001
Copy link
Member Author

There is realworld riscv64 hardware supporting the B extension,
https://www.sifive.com/cores/u74
and we can buy a dev-board from https://www.iceasy.com/10210/1022688923.shtml

So apparently the Starfive VisionFive board does not support the B extensions (even though it has U74 cores) - the specification on the allnetchina website (and others) only lists rv64gc and someone with access to one advises that the FDT also has a riscv,isa value of rv64imafdc.

I will buy this board and verify if the B instructions can run normally.

@benshi001
Copy link
Member Author

benshi001 commented Mar 23, 2022

I have consult my friends who has this dev-board, he confirmed that several B instructions such andn,orc,bclr would cause illegal instruction exception.

So I will close this proposal, sorry for bothering. The SiFive's website misguide me.

@benshi001 benshi001 moved this from Active to Declined in Proposals (old) Mar 23, 2022
@wdvxdr1123
Copy link
Contributor

Hi, U74 does not support full B instructions, actually U74 only support zba and zbb instructions as stated in the U74 complex manual.

I bought a Starfive VisionFive2 board which has quad u74 cores. I verified that both zba and zbb instructions can work correctly, but zbc and zbs instructions will cause a SIGILL.

@benshi001
Copy link
Member Author

Hi, U74 does not support full B instructions, actually U74 only support zba and zbb instructions as stated in the U74 complex manual.

I bought a Starfive VisionFive2 board which has quad u74 cores. I verified that both zba and zbb instructions can work correctly, but zbc and zbs instructions will cause a SIGILL.

You can implement zba and zbb first, and do some tiny optimizations like this,
llvm/llvm-project@5b6c9a5
llvm/llvm-project@264b8e2
llvm/llvm-project@d934b72

@wdvxdr1123
Copy link
Contributor

You can implement zba and zbb first, and do some tiny optimizations like this

I mailed https://go.dev/cl/461143, https://go.dev/cl/461144. I will try to do some optimizations and run benchmarks on dev board.

@golang golang locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Issues solely affecting the riscv64 architecture. FrozenDueToAge Proposal
Projects
No open projects
Development

No branches or pull requests

8 participants