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

[benshi001]

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?

[randall77]

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.

[benshi001]

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]

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]

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

[rsc]

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

[4a6f656c]

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]

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]

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

[rsc]

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

[benshi001]

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]

ping ...

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

[ianlancetaylor]

Moving this back to the incoming proposal queue.

[rsc]

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]

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]

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]

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

[benshi001]

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]

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]

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]

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]

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]

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]

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]

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]

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

[benshi001]

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]

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.

[wdvxdr1123]

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]

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]

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.