-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
Comments
(attn @golang/riscv64) |
See also #47373 (cc @benshi001, @wdvxdr1123) |
I agree that a
|
I think |
FYI There is one builder |
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? |
What is the set of values that should be accepted here? And what build tag or tags should be enabled for each value? |
This proposal has been added to the active column of the proposals project |
@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 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:
This is, AFAIU, the same approach taken by Go for the GOAMD64 variable. |
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. |
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? |
I'll change these to lower case in the table in the top post.
Yes |
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
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).
Runtime detection is preferable, but not workable for various aspects of compiler generated code (I suspect |
Thanks for the reply. I think I've captured the issues/concerns you've raised in the Open Issues section of the top post.
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:
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. |
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 |
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). |
What I said last week is also exactly what is at the top comment, so it sounds like we all agree. |
Based on the discussion above, this proposal seems like a likely accept. |
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. |
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. |
This proposal has been added to the active column of the proposals project |
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. |
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.
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. |
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:
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. |
This looks good. One question though, is there any reason to keep the riscv64.rv64g build tag, i.e., should the levels be?
|
My mistake - I meant to delete the riscv64.rv64g build tag too. Consider it done. |
Based on the discussion above, this proposal seems like a likely accept. 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:
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. |
No change in consensus, so accepted. 🎉 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:
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. |
I'll start working on the implementation. |
Change https://go.dev/cl/541135 mentions this issue: |
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:
Did I miss anything? |
This can wait until it is needed by some code.
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.
Probably not needed for an initial version.
Yep. |
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? |
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>
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? |
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. |
There are really some:
|
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. |
Change https://go.dev/cl/591895 mentions this issue: |
For #65614. Updates #61476. Change-Id: Id677aa6d2a59366ab75a26f08a383d2d253f270e Reviewed-on: https://go-review.googlesource.com/c/go/+/591895 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Joel Sing <joel@sing.id.au> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
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
Once RVA23U64 is ratified a fourth permissable value could be supported
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.
The text was updated successfully, but these errors were encountered: