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

all: add GORISCV64 environment variable #61476

Closed
markdryan opened this issue Jul 20, 2023 · 44 comments
Closed

all: add GORISCV64 environment variable #61476

markdryan opened this issue Jul 20, 2023 · 44 comments
Labels
arch-riscv Issues solely affecting the riscv64 architecture. Proposal Proposal-Accepted
Milestone

Comments

@markdryan
Copy link
Contributor

markdryan commented Jul 20, 2023

Overview

Go currently targets the RV64G instruction set when building for GOARCH=riscv64. This gives the compiler and hand-coded assembly language functions in the runtime and standard library access to scalar integer, multiply, divide, scalar floating point, atomic and fence instructions and some counters, but little else. RISC-V does define a number of extensions that could be used in the compiler (and the standard library) to improve performance and reduce code size. For example, the Bit Manipulation extensions could be used to speed up array access and implement an intrinsic for math/bits.OnesCount. The Vector extensions could be used to improve some of the hand-coded assembly language functions, such as memmove. However, the compiler cannot currently use those extensions without a runtime check (which it can't currently do either as it happens) as it needs to generate code that will run safely on all RV64G devices.

Proposal 61416 x/sys/unix, x/sys/cpu: use the RISC-V Hardware Probing Interface on Linux proposes a method of determining which extensions exist at runtime (on Linux). This proposal suggests a method to determine the existence of useful extensions at compile time. Similar mechanisms exists for other architectures supported by Go and are implemented via the use of an environment variable, e.g., GOAMD64. Here we propose adding a new RISC-V specific environment variable, GORISCV64, that could be used by the compiler and assembly language functions to determine which extensions can be safely used without runtime checks.

Suggested values for GORISCV64

RISC-V defines a set of profiles. Each of these profiles specify a base ISA and a set of mandatory extensions. For example, devices supporting the RVA20U64 profile must support RV64GC providing access to the compressed instruction set. Devices supporting RVA22U64 must additionally support Zba, Zbb and Zbs. RVA23U64 (which is not yet ratified) currently mandates the Vector extension and Pointer Masking.

This proposal recommends using the names of these standardised profiles, converted to lowercase, as valid values for the GORISCV64 environment variable, with one exception. There is no profile that corresponds to what the compiler currently targets, RV64G, so RV64G would also be a valid value of GORISCV64 and the default.

In summary, we would have

GORISCV64 Value buildcfg.GORISCV64 Build Tags Potential Benefits
rv64g 0 riscv64.rv64g The default and what Go currently targets
rva20u64 20 riscv64.rv64g, riscv64.rva20u64 Would allow the compiler to generate compressed instructions
rva22u64 22 riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64 Would allow the compiler to generate Zba, Zbb and Zbs instructions without runtime checks

Once RVA23U64 is ratified a fourth permissable value could be supported

GORISCV64 Value buildcfg.GORISCV64 Build Tags Potential Benefits
rva23u64 23 riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64 Would potentially allow the use of vector instructions in hand written assembler without runtime checks. Pointer masking might be useful.

This proposal just covers the addition of the new environment variable. Extra work would be needed to take advantage of this variable such as adding support in the assembler for the extensions mandated by the profiles and updating the compiler and standard library routines to use them. I'll enter separate issues for these items.

Open Issues

This section will capture issues raised in the discussion below that do not yet have an agreed resolution.

  • Should the default value of GORISCV64 be rv64g (what the compiler currently targets and isn't actually a profile) or rva20u64 which makes the compressed instruction set mandatory?
  • Should GORISCV64 be restricted to rv64g and the profile names only (or perhaps just the profile names) or should we allow users to specify a complete ISA string? Complete ISA strings allow individual extensions to be specified and can include profile names but introduce additional complexity.
  • The current builders only support rv64g and rva20u64. These builders do support some but not all of the mandatory extensions in rva22u64. Restricting GORISCV64 to profile names (and rv64g) only will delay improvements to the compiler that can take advantage of these extensions, notably, Zba and Zbb, until we have builders that support rva22u64.
@gopherbot gopherbot added this to the Proposal milestone Jul 20, 2023
@bcmills bcmills added the arch-riscv Issues solely affecting the riscv64 architecture. label Jul 20, 2023
@bcmills
Copy link
Contributor

bcmills commented Jul 20, 2023

(attn @golang/riscv64)

@4a6f656c
Copy link
Contributor

See also #47373 (cc @benshi001, @wdvxdr1123)

@4a6f656c
Copy link
Contributor

I agree that a GORISCV64 environment variable is likely needed and I think that it is now a reasonable point in time to introduce it. That said, some points to consider:

  • Runtime detection is preferred - as far as possible, we should prefer runtime detection over compile time selection - there are a number of use cases (e.g. cross compilation, binary package builds, etc) where the lower common denominator has to be selected, in order to work across all hardware/platforms. There are cases where this works (e.g. switching to a SHA512 implementation that uses specific instructions at runtime) and cases where it does not (e.g. compiler rewrite rules).

  • Compressed instructions - while it is true that Go currently targets rv64g, in reality there is probably no reason that this cannot be rv64gc which would presumably allow RVA20U64 as the base profile. I'm not aware of hardware/environments existing that meet rv64g but do not have c support - other compilers and operating systems target rv64gc as well (as an aside, I have various compressed instruction diffs floating around).

  • Profiles versus reality - one issue to consider is that while I think the use of profiles is a nice and clean approach, in reality this does not work well. For example, the Starfive VisionFive 2 has support for rv64gc however only includes zba and zbb. This means that on this hardware RVA22U64 could not be used since zbs is not supported. One previous suggestion was to use some form of feature list - most riscv64 environments provide this in a form like rv64imafdc_zba_zbb, which would be readily parsable. The specification of a profile would simply map to a similar string.

  • Builders and testing - adding new instructions and compiler rules that use additional extensions is problematic if the builders are not actually testing them. The current builders are SiFive HiFive Unleashed and SiFive HiFive Unmatched based, which are rv64imafdc (I don't have access to an Unmatched to confirm, but I believe it is the same as the Unleashed). I should be bringing a Starfive VisionFive 2 online for the openbsd/riscv64 builder soon, which would make zba and zbb available. Otherwise we probably need to consider additional builders and/or test environments that provide coverage for new code.

@wdvxdr1123
Copy link
Contributor

The current builders are SiFive HiFive Unleashed and SiFive HiFive Unmatched based

I think linux-riscv64-unmatched is actually running on Starfive VisionFive2 now. cc @mengzhuo

@mengzhuo
Copy link
Contributor

The current builders are SiFive HiFive Unleashed and SiFive HiFive Unmatched based

I think linux-riscv64-unmatched is actually running on Starfive VisionFive2 now. cc @mengzhuo

FYI There is one builder unmatched025.farm.rvlab.org based Unmatched and two VisionFive2 from my homelab, it's a temporary solution since "rvlab" is rebuilding their build farm.

@markdryan
Copy link
Contributor Author

  • Profiles versus reality - one issue to consider is that while I think the use of profiles is a nice and clean approach, in reality this does not work well. For example, the Starfive VisionFive 2 has support for rv64gc however only includes zba and zbb. This means that on this hardware RVA22U64 could not be used since zbs is not supported. One previous suggestion was to use some form of feature list - most riscv64 environments provide this in a form like rv64imafdc_zba_zbb, which would be readily parsable. The specification of a profile would simply map to a similar string.

While it is true that limiting GORISCV64 to profile names wouldn't help us generate better code for the existing SBCs on the market, RVA22U64 does seem to be gaining some traction. SiFive and T-HEAD have announced RVA22U64 compatible processors and Android is targeting RVA22U64 going forward.

The reason I proposed using profile names and nothing else (apart from RV64G) for valid values for GORISCV64 is that this solution is similar to what is used on other architectures supported by Go. There is also the observations in the RISC-V profiles document that the ability to target individual extensions can lead to a "combinatorial explosion in possible ISA choices" and that "long and unwieldy ISA strings [are] required to encode common sets of extensions, which will continue to grow as new extensions are defined."

I guess the question we need to answer is whether it's worth the additional complexity of handling arbitrary ISA strings so we can target at compile time existing boards such as the VisionFive2 or future boards that implement extensions that are optional in their supported profile? Or is the simpler solution of targeting the mandatory extensions of the profiles at compile time and detecting everything else at runtime sufficient?

@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

What is the set of values that should be accepted here? And what build tag or tags should be enabled for each value?
For x86 we have v1 v2 v3 and if you set GOAMD64=v3 it also sets the v2 build tag because v3 implies v2.
(Otherwise v2-aware code would not be used with v3, making v3 possibly slower than v2.)
Is there any such relationship here?

@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

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

@luhenry
Copy link

luhenry commented Aug 10, 2023

What is the set of values that should be accepted here? And what build tag or tags should be enabled for each value?

@markdryan is coming back from PTO next week, so let me just chime in before he can give a more in-depth answer.

The proposal is to use RISC-V profile names, with GORISCV64=RVA20U64 or GORISCV64=RVA22U64 for example, and a default to GORISCV64=RV64G. These map directly to existing RISC-V profiles, RVA20U64 and RVA22U64 respectively.

The build tag enabled by each of this value would match directly the mandatory extensions defined by each profile. For RVA22U64, it would map to enabling RV64GC + Zba + Zbb + Zbs + Zihintpause + Zicbop + Zicboz + Zicbom + Zfhmin + Zkt. Note that not all of these extensions are currently taken advantage of in Go, but we are working on doing just that at the moment.

The motivation behind using profiles is already explicitely outlined in the RISC-V profile spec:

Profiles specify a much smaller common set of ISA choices that capture the most value for most users, and which thereby enable the software community to focus resources on building a rich software ecosystem with application and operating system portability across different implementations

This is, AFAIU, the same approach taken by Go for the GOAMD64 variable.

@markdryan
Copy link
Contributor Author

And what build tag or tags should be enabled for each value?
For x86 we have v1 v2 v3 and if you set GOAMD64=v3 it also sets the v2 build tag because v3 implies v2.
(Otherwise v2-aware code would not be used with v3, making v3 possibly slower than v2.)
Is there any such relationship here?

This is an interesting question. Newer profiles are not precisely supersets of the older profiles. For example the draft RVA23U64 profile drops support for Zkn and Zks which are optional in RVA22U64 and the minimum reservation set changes from 128 bytes in RVA20U64 to 64 bytes in RVA22U64. In addition, the profiles documentation does not make any statements about forward compatibility that I can see (I've entered an issue asking for clarification).

However, if you stick to the mandatory baselines and the mandatory extensions defined by the profiles, i.e., the ones that are useful at compile time, and you don't write code that breaks if the reservation set size changes, then there is a clear relationship between the existing profiles, i.e., all the instructions that are mandatory in RVA20U64 are mandatory in both RVA22U64 and RVA23U64 and those mandatory instructions added in RVA22U64 are also mandatory in RVA23U64.

I will update the proposal above to document the build tags.

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

Probably the GORISCV64 setting should be lowercase like the build tags to match everything else in Go.

So it sounds like the four settings are rv64g, rva20u64, rva22u64, and rva23u64, and a setting later in that list enables the build tags that precede it as well (with a riscv64. prefix on all the build tags like riscv64.rv64g, ...).

Do I have that right?

@markdryan
Copy link
Contributor Author

Probably the GORISCV64 setting should be lowercase like the build tags to match everything else in Go.

I'll change these to lower case in the table in the top post.

So it sounds like the four settings are rv64g, rva20u64, rva22u64, and rva23u64, and a setting later in that list enables the build tags that precede it as well (with a riscv64. prefix on all the build tags like riscv64.rv64g, ...).

Do I have that right?

Yes

@4a6f656c
Copy link
Contributor

  • Profiles versus reality - one issue to consider is that while I think the use of profiles is a nice and clean approach, in reality this does not work well. For example, the Starfive VisionFive 2 has support for rv64gc however only includes zba and zbb. This means that on this hardware RVA22U64 could not be used since zbs is not supported. One previous suggestion was to use some form of feature list - most riscv64 environments provide this in a form like rv64imafdc_zba_zbb, which would be readily parsable. The specification of a profile would simply map to a similar string.

While it is true that limiting GORISCV64 to profile names wouldn't help us generate better code for the existing SBCs on the market, RVA22U64 does seem to be gaining some traction. SiFive and T-HEAD have announced RVA22U64 compatible processors and Android is targeting RVA22U64 going forward.

To be clear, I'm all for using profile names - that is the clean and sensible approach. I'm less concerned about targeting existing SBCs and more concerned about being able to test Go on the builder hardware that is currently available. The current situation is one where Go could generate zba and zbb code, but it would not be tested by the builders (even though some of them could).

The reason I proposed using profile names and nothing else (apart from RV64G) for valid values for GORISCV64 is that this solution is similar to what is used on other architectures supported by Go. There is also the observations in the RISC-V profiles document that the ability to target individual extensions can lead to a "combinatorial explosion in possible ISA choices" and that "long and unwieldy ISA strings [are] required to encode common sets of extensions, which will continue to grow as new extensions are defined."

To reiterate, we should make the default rv64gc rather than rv64g - I believe that we are going to make it much harder to benefit from compressed instructions if we explicitly exclude it from the baseline in this proposal. I'm not aware of any available hardware that supports rv64g but does not support c (and AIUI this matches what gcc and llvm have targeted).

I guess the question we need to answer is whether it's worth the additional complexity of handling arbitrary ISA strings so we can target at compile time existing boards such as the VisionFive2 or future boards that implement extensions that are optional in their supported profile? Or is the simpler solution of targeting the mandatory extensions of the profiles at compile time and detecting everything else at runtime sufficient?

Runtime detection is preferable, but not workable for various aspects of compiler generated code (I suspect zb* fall into this category, for example).

@markdryan
Copy link
Contributor Author

Thanks for the reply. I think I've captured the issues/concerns you've raised in the Open Issues section of the top post.

To be clear, I'm all for using profile names - that is the clean and sensible approach. I'm less concerned about targeting existing SBCs and more concerned about being able to test Go on the builder hardware that is currently available. The current situation is one where Go could generate zba and zbb code, but it would not be tested by the builders (even though some of them could).

Is your preference then to allow the GORISCV64 variable to accept a mixture of profiles and extensions, for example, GORISCV64=rva20u64_zba_zbb or GORISCV64="rva20u64,zba,zbb", rather than just profiles? If so, it might be useful to have a table, similar to the one we have in the top post, describing this alternative to help us understand what it would look like, in terms of build tags and buildcfg.GORISCV64. I can add such a table to the issue next week.

@4a6f656c
Copy link
Contributor

Thanks for the reply. I think I've captured the issues/concerns you've raised in the Open Issues section of the top post.

To be clear, I'm all for using profile names - that is the clean and sensible approach. I'm less concerned about targeting existing SBCs and more concerned about being able to test Go on the builder hardware that is currently available. The current situation is one where Go could generate zba and zbb code, but it would not be tested by the builders (even though some of them could).

Is your preference then to allow the GORISCV64 variable to accept a mixture of profiles and extensions, for example, GORISCV64=rva20u64_zba_zbb or GORISCV64="rva20u64,zba,zbb", rather than just profiles? If so, it might be useful to have a table, similar to the one we have in the top post, describing this alternative to help us understand what it would look like, in terms of build tags and buildcfg.GORISCV64. I can add such a table to the issue next week.

Thanks - I think there are three possibilities here:

  1. We only allow profiles and do not accept functionality for a new profile until a builder exists with hardware for that profile.
  2. We only allow profiles and accept that Go has code that is not being tested on builders (which potentially leads to undetected breakage).
  3. We allow a profile and comma separated list of additional extensions, which would allow us to add code for an extension that an existing builder could test.

I believe (1) is likely preferable, but it is also going to block progress with a new profile until such time as builder hardware becomes available.

@4a6f656c
Copy link
Contributor

I've confirmed that it is going to be difficult to support compressed instructions in a later profile, if we do not in the base profile - in order to add compressed instruction support we need to reduce the PC quantum, which also means having a NOP instruction that is the same length as the quantum (at least, without making changes to a current runtime requirement - see runtime.goexit). And without compressed instructions there is no two byte NOP sequence...

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

It seems like we can start with these base profiles - rv64g, rva20u64, rva22u64, and rva23u64 - and add individual features later in separate proposals as we find needs for them. Compressed instructions also seem orthogonal to these base profiles, since if we add them we can have a build tag for that and set the PC quantum correctly based on that build tag (or we could just hard-code PC quantum to 2 always).

@rsc
Copy link
Contributor

rsc commented Sep 6, 2023

What I said last week is also exactly what is at the top comment, so it sounds like we all agree.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2023

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

@4a6f656c
Copy link
Contributor

I disagree that compressed instruction support is strictly orthogonal.

If we move forward and explicitly make the base profile one that excludes compressed instructions, then we're unnecessarily creating significant hurdles for their implementation (I can elaborate further at some point if needed, but the key issues were mentioned earlier).

On the other hand, making rva20u64 the base profile means that all values are real profiles and we avoid adding unnecessary complexity and hurdles for compressed instruction support. And as far as I can see there is no downside to doing this.

@rsc
Copy link
Contributor

rsc commented Sep 18, 2023

Thanks @4a6f656c. It sounds like maybe the situation here with compressed instructions is not as clear as we thought. Probably should move back to Active at the next proposal review meeting.

@rsc
Copy link
Contributor

rsc commented Oct 3, 2023

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
Copy link
Contributor

rsc commented Oct 9, 2023

A correspondent emailed me a link to this recent presentation to RISC-V International titled "A Case to Remove C from App Profiles", along with this public mailing list discussion where the slides are linked. It is unclear to me what exactly this means for our decision here. It sounds like adopting the names we discussed above would be fine but we should probably hold back from actually using compressed instructions for now.

@markdryan
Copy link
Contributor Author

A correspondent emailed me a link to this recent presentation to RISC-V International titled "A Case to Remove C from App Profiles", along with this public mailing list discussion where the slides are linked.

Thanks for posting this. The proposal linked to above recommends adding a new set of RISC-V application profiles that would not support the compressed instruction set and instead would re-use the encoding space of the existing compressed instructions to support new 32 bit instructions. These new profiles would be incompatible with all the existing application profiles (RVA20U64, RVA22U64 and the unratified RVA23U64) as the existing profiles mandate support for compressed instructions. There are then two implications for this github issue.

  1. One of the open issues linked in the top post discusses what the default profile targeted by the Go compiler should be.
    Should it be RV64G or RVA20U64 (which would mandate compressed instructions)? If the proposal to create new profiles without compressed instructions were accepted it's possible future devices implementing those profiles would not support compressed instructions. If Go were to target RVA20U64 as the base profile, Go programs may not be able to run on those devices. So it's probably best if Go continues to target RV64G as the base profile for the time being.

  2. The proposal in this github issue assumes that any new RISC-V profiles will be backwardly compatible with the existing profiles. This allows the compiler to check one integer value (buildcfg.GORISCV64) to determine whether an extension is supported and files that rely on a RISC-V extension to contain a single build tag associated with the profile where the extension was first made mandatory. This simple solution breaks down when there are parallel sets of incompatible RISC-V profiles. Existing code that used a profile derived build tag or checked buildcfg.GORISCV64 would need to be updated when support for the new incompatible profiles were added.

So how should we proceed? I suggest updating the original proposal in this issue so that it copes better with the emergence of any future incompatible sets of profiles. The basic idea would be to still use the profile names as the valid values for GORISCV64 (RV64G, RVA20U64, RVA22U64) but to use more fine grained build tags derived from the RISC-V extension names mandated by the profiles and a more complex implementation of buildcfg.GORISCV64 that would allow the compiler to determine which extensions are supported rather than which profiles. In this scheme the emergence of new sets of profiles would not require modification of existing code, even if those profiles were incompatible with those already supported by Go. This is similar to what was proposed above with the exception that GORISCV64 would be restricted to profile names and RV64G.

The alternative is to wait to see what happens with the "A Case to Remove C from App Profiles" proposal but my feeling is that the fact it exists and that it breaks the original proposal in this github issue, demonstrates that the original proposal was too inflexible.

@rsc
Copy link
Contributor

rsc commented Oct 10, 2023

I'm not sure the proposal breaks what we have been discussing. In particular, the fact that internal/buildcfg.GORISCV64 could be an integer is an internal API that can be changed as needed in the future. It is not a detail we are locked into. We can put a compressed-or-not bit in the integer or make it a struct or anything else we want. No one will be able to tell.

The details we'd be locked into are the strings acceptable in the GORISCV64 environment variable and the //go:build tags that are present for each variable. For those, the table in the top post still seems to apply. Specifically:

  • GORISCV64=rv64g: build tag riscv64.rv64g only
  • GORISCV64=rva20u64: build tags riscv64.rv64g, riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

The question I have about the presentation is whether these profiles would be retroactively redefined to disallow compressed instructions. Go can be conservative and simply not emit compressed instructions until the dust settles.

@markdryan
Copy link
Contributor Author

Actually, I do have one more observation. The only additional instructions enabled by RVA20U64 over RV64G are the compressed instructions. Setting GORISCV64=rva20u64 will not have any affect until compressed instructions are implemented. Does it still make sense to support rva20u64 right now or should we limit the valid values of GORISCV64 to just rv64g and rva22u64 (which gives us Zba, Zbb and Zbs) for the time being?

@rsc rsc changed the title proposal: all: Add GORISCV64 environment variable proposal: all: add GORISCV64 environment variable Oct 12, 2023
@rsc
Copy link
Contributor

rsc commented Oct 12, 2023

Since we kind of made up the name rv64g, instead of dropping rva20u64, perhaps we should drop rv64g. If at some point we add compressed instructions, we can add a ',compress' or ',nocompress' or something like that to the value.

@rsc
Copy link
Contributor

rsc commented Oct 12, 2023

My summary of the proposal would now be:

Following GOARM, GOAMD64, and so on, we define a GORISCV64 variable to specify the architecture level that can be assumed by the compiler and (using //go:build tags) in Go packages.

The three levels are:

  • GORISCV64=rva20u64: build tags riscv64.rv64g, riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

There is uncertainty about the future of compression in RISC-V. For now, even if we are using one of these later architecture levels, we will continue not to emit compressed instructions.

Perhaps at some point in the future we would add a “,compress” suffix to ask the toolchain to generate compressed instructions, like in GORISCV64=rva20u64,compress. Because every compressed instruction has an uncompressed instruction with identical semantics, “compress or not” can be handled entirely inside the toolchain, without exposing that detail to assembly files or in build tags.

@markdryan
Copy link
Contributor Author

My summary of the proposal would now be:

Following GOARM, GOAMD64, and so on, we define a GORISCV64 variable to specify the architecture level that can be assumed by the compiler and (using //go:build tags) in Go packages.

The three levels are:

  • GORISCV64=rva20u64: build tags riscv64.rv64g, riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rv64g, riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

There is uncertainty about the future of compression in RISC-V. For now, even if we are using one of these later architecture levels, we will continue not to emit compressed instructions.

Perhaps at some point in the future we would add a “,compress” suffix to ask the toolchain to generate compressed instructions, like in GORISCV64=rva20u64,compress. Because every compressed instruction has an uncompressed instruction with identical semantics, “compress or not” can be handled entirely inside the toolchain, without exposing that detail to assembly files or in build tags.

This looks good. One question though, is there any reason to keep the riscv64.rv64g build tag, i.e., should the levels be?

  • GORISCV64=rva20u64: build tags riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

@rsc
Copy link
Contributor

rsc commented Oct 24, 2023

My mistake - I meant to delete the riscv64.rv64g build tag too. Consider it done.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2023

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

The proposal details are as follows.

Following GOARM, GOAMD64, and so on, we define a GORISCV64 variable to specify the architecture level that can be assumed by the compiler and (using //go:build tags) in Go packages.

The three levels are:

  • GORISCV64=rva20u64: build tags riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

There is uncertainty about the future of compression in RISC-V. For now, even if we are using one of these later architecture levels, we will continue not to emit compressed instructions.

Perhaps at some point in the future we would add a “,compress” suffix to ask the toolchain to generate compressed instructions, like in GORISCV64=rva20u64,compress. Because every compressed instruction has an uncompressed instruction with identical semantics, “compress or not” can be handled entirely inside the toolchain, without exposing that detail to assembly files or in build tags.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Following GOARM, GOAMD64, and so on, we define a GORISCV64 variable to specify the architecture level that can be assumed by the compiler and (using //go:build tags) in Go packages.

The three levels are:

  • GORISCV64=rva20u64: build tags riscv64.rva20u64
  • GORISCV64=rva22u64: build tags riscv64.rva20u64, riscv64.rva22u64
  • GORISCV64=rva23u64: build tags riscv64.rva20u64, riscv64.rva22u64, riscv64.rva23u64

There is uncertainty about the future of compression in RISC-V. For now, even if we are using one of these later architecture levels, we will continue not to emit compressed instructions.

Perhaps at some point in the future we would add a “,compress” suffix to ask the toolchain to generate compressed instructions, like in GORISCV64=rva20u64,compress. Because every compressed instruction has an uncompressed instruction with identical semantics, “compress or not” can be handled entirely inside the toolchain, without exposing that detail to assembly files or in build tags.

@rsc rsc changed the title proposal: all: add GORISCV64 environment variable all: add GORISCV64 environment variable Nov 2, 2023
@rsc rsc modified the milestones: Proposal, Backlog Nov 2, 2023
@markdryan
Copy link
Contributor Author

I'll start working on the implementation.

@gopherbot
Copy link

Change https://go.dev/cl/541135 mentions this issue: cmd/go: add GORISCV64 environment variable

@markdryan
Copy link
Contributor Author

As RVA23U64 is not yet ratified,  https://go.dev/cl/541135 only implements GORISCV64=rva20u64 and GORISCV64=rva22u64.  Support for GORISCV64=rva23u64 can be added later, once it is ratfiied.

Looking at the existing support for GOAMD64, there are still some things missing for GORISCV64:

  • We're going to need something like asm_amd64.h for riscv64 to define assembler visible macros for the various extensions.  It seemed a bit premature to add this now as none of the extensions we'd define macros for are supported by the assembler yet. However, I can update the patch if people would like to see this implemented now.
  • For amd64 binaries, the runtime checks that the device on which it runs supports the value of GOAMD64 that it was compiled with.  There's a similar check for GOARM.  It would be nice to do something like this for riscv64, but this will require the ability to detect extensions at runtime.  Hopefully, this could be done for Linux with the help of https://go-review.googlesource.com/c/go/+/522995 after some reworking.
  • GOARCH=amd64 has a test to verify that instructions in the feature levels v2 and above are not used by the compiler when GOAMD64=v1.  The other architectures don't seem to have a similar test.  We could potentially add one for riscv64 once we have some code that uses the extensions in rva22u64 and above.
  • Ultimately, we're going to need a builder that supports rva22u64 (and above).

Did I miss anything?

@randall77
Copy link
Contributor

We're going to need something like asm_amd64.h for riscv64

This can wait until it is needed by some code.

For amd64 binaries, the runtime checks that the device on which it runs supports the value of GOAMD64 that it was compiled with.

This would be good to have. In the worst case, we can just issue one of every instruction family, assuming they will SIGILL and not be simulated by the OS or anything like that. That way, the program dies immediately on startup and not when it reaches for a missing instruction sometime later. The error message won't be as nice as we could make it with proper detection code, but at least it will exist.

GOARCH=amd64 has a test to verify that instructions in the feature levels v2 and above are not used by the compiler when GOAMD64=v1. The other architectures don't seem to have a similar test. We could potentially add one for riscv64 once we have some code that uses the extensions in rva22u64 and above.

Probably not needed for an initial version.

Ultimately, we're going to need a builder that supports rva22u64 (and above).

Yep.

@MouseSplinter
Copy link
Contributor

A correspondent emailed me a link to this recent presentation to RISC-V International titled "A Case to Remove C from App Profiles", along with this public mailing list discussion where the slides are linked. It is unclear to me what exactly this means for our decision here. It sounds like adopting the names we discussed above would be fine but we should probably hold back from actually using compressed instructions for now.

Hi, Cox. It seems that the RISCV BoD had made decision about the compressed instructions of RVA profiles. Maybe it's time to support it?

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
The variable represents the RISC-V user-mode application profile for
which to compile.  Valid values are rva20u64 (the default) and
rva22u64.

Setting GORISCV64=rva20u64 defines the riscv64.rva20u64 build tag,
sets the internal variable buildcfg.GORISCV64 to 20 and defines the
macro GORISCV64_rva20u64 for use in assembly language code.

Setting GORISCV64=rva22u64 defines the riscv64.rva20u64 and
riscv64.rva22u64 build tags, sets the internal variable
buildcfg.GORISCV64 to 22 and defines the macro GORISCV64_rva22u64
for use in assembly language code.

This patch only provides a mechanism for the compiler and hand-coded
assembly language functions to take advantage of the RISC-V
extensions mandated by the application profiles.  Further patches
will be required to get the compiler/assembler and assembly language
functions to actually generate and use these extensions.

Fixes golang#61476

Change-Id: I9195ae6ee71703cd2112160e89157ab63b8391af
Reviewed-on: https://go-review.googlesource.com/c/go/+/541135
Reviewed-by: M Zhuo <mengzhuo1203@gmail.com>
Reviewed-by: Joel Sing <joel@sing.id.au>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Wang Yaduo <wangyaduo@linux.alibaba.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: M Zhuo <mengzhuo1203@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@Xeonacid
Copy link

Xeonacid commented Feb 20, 2024

RV64GC is not equivalent to RVA20U64.

For example, Zicclsm supports "Misaligned loads and stores".

Should we support those extensions? Or do we write RVA20U64 but actually only use RV64GC?

@markdryan
Copy link
Contributor Author

markdryan commented Feb 20, 2024

Should we support those extensions? Or do we write RVA20U64 but actually only use RV64GC?

My understanding is that the Go compiler/runtime currently generates/uses IMAFD_Zicsr_Zicntr instructions ( a subset of those defined by RVA20U64 ) regardless of what profile is selected. I believe it assumes Ziccamoa and Ziccrse are also supported.

What would supporting the rest of the mandatory extensions you list that are present in RVA20U64 but not in RV64GC actually entail? None of these extensions add any instructions. Zicclsm would allow us to perform misaligned loads and stores, but as the profile spec notes, these unaligned loads and stores may be very slow, and indeed they are on the hardware we currently test on such as the VisionFive2, and so should be avoided. Misaligned loads and stores could potentially be used on Linux after a runtime check to the hwprobe sys call, but could not I think, be enabled by profile selection, at least with the currently defined profiles.

@Xeonacid
Copy link

What would supporting the rest of the mandatory extensions you list that are present in RVA20U64 but not in RV64GC actually entail? None of these extensions add any instructions.

There are really some: fence.tso and Zicntr (rdcycle, rdtime and rdinstret).

The fence.tso instruction was incorrectly described as optional in the 2019 ratified specifications. However, fence.tso is encoded within the standard fence encoding such that implementations must treat it as a simple global fence if they do not natively support TSO-ordering optimizations. As software can always assume without any penalty that fence.tso is being exploited by a hardware implementation, there is no advantage to making the instruction a profile option. Later versions of the unprivileged ISA specifications correctly indicate that fence.tso is mandatory.

@markdryan
Copy link
Contributor Author

There are really some: fence.tso and Zicntr (rdcycle, rdtime and rdinstret).

fence.tso is part of the base ISA so isn't really relevant to the profiles discussion. It is already supported by the assembler I believe, although I don't see it used anywhere. Zicntr is also supported by the assembler and is already used by the runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv Issues solely affecting the riscv64 architecture. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests