[RFC] zicntr and zihpm issues and how to deal with future ISA spec changes in future?

[Kito Cheng]

Hi all:

I'd like to reach out to the community to get a better understanding
of how we should handle ISA spec changes in the future. This topic
that has been discussed a few years ago [1]. However, as time passed
by, I think it is fair to raise the topic again.

# What happened?

Recent proposals in the profiles specification represent a change of
the ISA specification in an incompatible way: the new specification
drops two features from the base ISA that were there before. The two
features that are dropped from the base ISA are now available via the
two (new introduced) ISA extensions zicntr and zihpm. This issue was
discovered by Jim earlier this year ([2]).


To get an understanding of the two extensions:

* zicntr covers cycle[h], time[h], instret[h] (which did not belong to
any extension before)
* zihpm covers hpmcounter0[h] - hpmcounter31[h] (which did not belong
to any extension before)


Of course, the complete picture is not that simple. The two features
were intended to be ISA extensions in the past, but this was not done
(see [2]).


# What’s the problem?

Currently, the Toolchain projects restrict the available features by
parsing the march-string (provided via the -march=... option). The
availability of features is then provided to internal code generation
functionality (which will bail out if not-available features are
requested) and to the provided code (via test macros like __riscv_m).

When looking at zicntr (zihpm is similar) we are facing the following
situation: the user needs to specify "zicntr" as part of the march
string (e.g. -march=rv64i_zicntr) and the user can write test code
that requires the __riscv_zicntr macro to be defined in order to use
the zicntr functionality.

However, code that has been written until now, will compile without
"zicntr" as part of the march string and the code will not test for
the availability of __riscv_zicntr.

The impact is:

* incompatible behavior: existing build scripts will not work with
recent toolchains (zicntr is missing in the march string)
* reduced support of old compilers: new code that uses the
__riscv_zicntr macros will not work with old compilers that don't
support zicntr, even when the functionality exists

# What options do we have?

## Options 1: Ignore

Use the same rules as other extensions and gate the access to the
functionality by the march strings:

- The CSRs are only available when zicntr or zihpm are enabled.
- __riscv_zicntr/__riscv_zihpm are only defined when zicntr/zihpm are enabled.

Pros: Consistent behavior with other extensions
Cons: Incompatible change (as discussed above)


Option 2: who cares about these details

Make the CSRs always available, even when zicntr or zihpm are not
specified in the march string. Expose the test macros based on the
availability of the extension (via march string).

Pros: No compatibility issue.
Cons: Unexpected behavior (CSRs are available although the compiler
knows better); potentially harmful (compiler errors are expected to
prevent unsupported code from being generated);

## Option 3: use -misa-spec to decide what to do

Additionally to the flag -march=... the tools also have the flag
-misa-spec to control the behavior of the extension parser. Currently,
the misa-spec string can be "2.2" | "20190608" | "20191213". That flag
is also, where would add the new profiles ("rv20" and "rv22").

So for the existing misa-spec strings we would keep the current
behavior, but for the new "rv20" and "rv22" strings, we would change
the behavior.

Pros: we don't try to solve a problem that is not tool-specific but
ISA-specific; we have an option to get the old functionality back (the
default misa-spec can be set when building a compiler using
"--with-isa-spec=...")
Cons: Users need to be made aware of the issue; more work with
processing error reports from users

## Option 4: something else?

The three options above are not the only existing solutions. If you
are interested in some other solution, please share your thoughts!

# References

[1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ
[2] https://github.com/riscv/riscv-profiles/issues/43

[Christoph Müllner]

Hi Kito,

I prefer option #3, as I think we should not solve any incompatibilities between different versions of the ISA spec.

If the spec versions are not compatible, we (the tools authors) should support the changes as transparently as possible.
We should not hide facts from users. If they don't want to see the new (potentially breaking) behavior, they can always
use -misa-spec to get a stable behavior (and no new features).

The reason is, that any incompatible changes that the spec authors decide upon have to be assumed to be done with
good reasons and after the necessary discussions (RVI has a good ratification process that enforces this - e.g. during public review).
Of course, the issue should be reported as early as possible so the spec authors can react to it (this has happened here).

BR

Christoph

[Christoph Müllner]

On Fri, May 27, 2022 at 9:11 AM Philipp Tomsich <philipp...@vrull.eu> wrote:

KIto,

Thanks for summarizing this.

As discussed in one of the last psABI meetings, the option best
mirroring the behavior intended by the ISA specifications is "Option
3: use -misa-spec to decide what to do": unfortunately, the different
ISA specification versions define a different "expansion from string
to features" and thus we need to qualify the architecture-string
(i.e., march=) in conjunction with the specification version (i.e.
misa-spec=).

I've discussed this with Kito the last days (offline).

We already have such a connection between the extensions and the ISA releases in Binutils.

So we might consider a better synchronization of the parsers of the tools.

And we might want to standardize the entries in the riscv-toolchains-conventions documentation.

However, that is independent of the actual issue that Kito presents here, therefore I'll create a new thread

later today that will cover this and include a proposal to integrate and standardize the profiles in the tools.

 

A few more comments inline below.

Given that these CSRs will not be widely accessed (i.e. mainly in
software running at higher privilege modes), we might get away with
breaking backward compatibility in this specific case.
However, this does probably not hold for future changes that may
impact widely visible features — so whatever solution we decide on at
this time, should be generally applicable.

> Option 2: who cares about these details
>
> Make the CSRs always available, even when zicntr or zihpm are not
> specified in the march string. Expose the test macros based on the
> availability of the extension (via march string).
>
> Pros: No compatibility issue.
> Cons: Unexpected behavior (CSRs are available although the compiler
> knows better); potentially harmful (compiler errors are expected to
> prevent unsupported code from being generated);
>
> ## Option 3: use -misa-spec to decide what to do
>
> Additionally to the flag -march=... the tools also have the flag
> -misa-spec to control the behavior of the extension parser. Currently,
> the misa-spec string can be "2.2" | "20190608" | "20191213". That flag
> is also, where would add the new profiles ("rv20" and "rv22").
>
> So for the existing misa-spec strings we would keep the current
> behavior, but for the new "rv20" and "rv22" strings, we would change
> the behavior.

From my understanding this does not entirely reflect the upstream
specification changes: any specification version that qualifies these
as explicitly gated by these new extension names will have to require
the changed behavior (i.e., not just the new profiles).  Consider the
profiles as a fancy name (or a macro) expanding to a single well-known
misa-spec= and march= combination: besides using the profiles, users
will still be able to construct different misa-spec/march pairs and we
will need to process them appropriately.

> Pros: we don't try to solve a problem that is not tool-specific but
> ISA-specific; we have an option to get the old functionality back (the
> default misa-spec can be set when building a compiler using
> "--with-isa-spec=...")
> Cons: Users need to be made aware of the issue; more work with
> processing error reports from users
>
> ## Option 4: something else?
>
> The three options above are not the only existing solutions. If you
> are interested in some other solution, please share your thoughts!
>
> # References
>
> [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ
> [2] https://github.com/riscv/riscv-profiles/issues/43

Thanks,
Philipp.

[Andrew Waterman]

The RISC-V architects have long been clear that this is the only answer that we think is sensible. (It almost seems like you are going to these mailing lists because you didn’t like the answer we provided in other forums…)

We only approved these specific backwards-incompatible changes on the basis that they will, in fact, “just work” in practice. We wouldn’t have done so for extensions that compilers actually need to target.

## Option 3: use -misa-spec to decide what to do

Additionally to the flag -march=... the tools also have the flag
-misa-spec to control the behavior of the extension parser. Currently,
the misa-spec string can be "2.2" | "20190608" | "20191213". That flag
is also, where would add the new profiles ("rv20" and "rv22").

So for the existing misa-spec strings we would keep the current
behavior, but for the new "rv20" and "rv22" strings, we would change
the behavior.

Pros: we don't try to solve a problem that is not tool-specific but
ISA-specific; we have an option to get the old functionality back (the
default misa-spec can be set when building a compiler using
"--with-isa-spec=...")
Cons: Users need to be made aware of the issue; more work with
processing error reports from users

## Option 4: something else?

The three options above are not the only existing solutions. If you
are interested in some other solution, please share your thoughts!

# References

[1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ
[2] https://github.com/riscv/riscv-profiles/issues/43

--
You received this message because you are subscribed to the Google Groups "RISC-V SW Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sw-dev+un...@groups.riscv.org.
To view this discussion on the web visit https://groups.google.com/a/groups.riscv.org/d/msgid/sw-dev/CALLt3TjDmJXT-0frrczpA94cCEwBm-J31T1JxBsGNLB7wpUMAg%40mail.gmail.com.

[Jim Wilson]

On Fri, May 27, 2022 at 6:37 AM Andrew Waterman <and...@sifive.com> wrote:

On Thu, May 26, 2022 at 11:19 PM Kito Cheng <kito....@sifive.com> wrote:

Option 2: who cares about these details

Make the CSRs always available, even when zicntr or zihpm are not
specified in the march string. Expose the test macros based on the
availability of the extension (via march string).

Pros: No compatibility issue.
Cons: Unexpected behavior (CSRs are available although the compiler
knows better); potentially harmful (compiler errors are expected to
prevent unsupported code from being generated);

The RISC-V architects have long been clear that this is the only answer that we think is sensible. (It almost seems like you are going to these mailing lists because you didn’t like the answer we provided in other forums…)

We only approved these specific backwards-incompatible changes on the basis that they will, in fact, “just work” in practice. We wouldn’t have done so for extensions that compilers actually need to target.

It is confusing for end users if the compiler handles different ISA extensions in different ways.  I think zicntr should work exactly the same as M.   If M is enabled, then the assembler accepts multiply instructions, the compiler generates multiply instructions, and the compiler defines __riscv_m so that user code can check if multiply is safe to use in an extended asm in C code or in assembly code.  If M is not enabled, then the assembler rejects multiply instructions, the compiler does not generate them, and __riscv_m is not defined so user code knows not to use multiply instructions.  zicntr should work exactly the same way.

Your argument is that because the compiler can't generate a rdcycle instruction, we don't need the same support.  But there are 3 issues here, and the other two are still relevant.  The assembler needs to know whether to accept a rdcycle instruction, and the compiler needs to define __riscv_zicntr so that user code knows if it can use rdcycle.  Compile time errors are much friendlier than run time errors.  Accepting invalid instructions and then trapping at run time is not a good choice.

Examples of programs affected by zicntr include riscv-tests/benchmarks/dhrystone and the perf code in the linux kernel.

Consider the riscv-tests dhrystone code.  If we add a check for __riscv_zicntr then we break support for old compilers that don't define it.  If we add zicntr to the -march option then we break support for old compilers that don't accept that.  If we don't add zinctr to march then we break support for new compilers that do support it.  We could add a -misa-spec option to force use of 2019 ISA specs that don't have zicntr but then we can't test any new ISA extensions.  To fix this, we could add configure support that checks the compiler version and decides whether to add zicntr to -march or not depending on the compiler version.  This is inconvenient.  The linux kernel is going to have a similar problem.

Another issue here is that this is a "zi" extension, and anything "zi" is clearly in the compiler domain as part of the unpriv spec.  If you named this sicntr for instance then I wouldn't care as much, as s extensions are outside the compiler domain.  End users know (or should know) that there is little or no point in adding s extensions to the compiler -march option.  It would be easy to explain and understand if the compiler handled z extensions differently from s extensions.  There is a bit of a convention break in that rdcycle is in the unpriv spec, and s is supposed to be used for priv spec extensions, but we can fix that with a bit of hand waving.  So if this was sicntr I might be OK with the compiler ignoring it.  And maybe zicsr could be sicsr but it is a little late for that, as we already added zicsr support to the toolchains.  It might be confusing to have zicsr and sicntr though which argues for changing zicsr to sicsr.  If we rename zicsr to sicsr we have a backward compatibility break.  If we add zicntr we have a backward compatibility break.  If we have sicntr and zicsr then there is no backward compatibility break, but we have confusing and inconsistent extension naming.  But if the compiler does ignore sicntr/sicsr extensions, then end users will have to accept that sometimes they get run time traps when they accidentally use the wrong instruction, because the compiler isn't checking that.

JIm

[Andrew Waterman]

On Fri, May 27, 2022 at 9:13 AM Jim Wilson <jim.wil...@gmail.com> wrote:

On Fri, May 27, 2022 at 6:37 AM Andrew Waterman <and...@sifive.com> wrote:

On Thu, May 26, 2022 at 11:19 PM Kito Cheng <kito....@sifive.com> wrote:

Option 2: who cares about these details

Make the CSRs always available, even when zicntr or zihpm are not
specified in the march string. Expose the test macros based on the
availability of the extension (via march string).

Pros: No compatibility issue.
Cons: Unexpected behavior (CSRs are available although the compiler
knows better); potentially harmful (compiler errors are expected to
prevent unsupported code from being generated);

The RISC-V architects have long been clear that this is the only answer that we think is sensible. (It almost seems like you are going to these mailing lists because you didn’t like the answer we provided in other forums…)

We only approved these specific backwards-incompatible changes on the basis that they will, in fact, “just work” in practice. We wouldn’t have done so for extensions that compilers actually need to target.

It is confusing for end users if the compiler handles different ISA extensions in different ways.  I think zicntr should work exactly the same as M.   If M is enabled, then the assembler accepts multiply instructions, the compiler generates multiply instructions, and the compiler defines __riscv_m so that user code can check if multiply is safe to use in an extended asm in C code or in assembly code.  If M is not enabled, then the assembler rejects multiply instructions, the compiler does not generate them, and __riscv_m is not defined so user code knows not to use multiply instructions.  zicntr should work exactly the same way.

Your argument is that because the compiler can't generate a rdcycle instruction, we don't need the same support.

Yes, that’s the extent of my argument. And I stand by it.

[Christoph Müllner]

On Fri, May 27, 2022 at 6:18 PM Andrew Waterman <and...@sifive.com> wrote:

On Fri, May 27, 2022 at 9:13 AM Jim Wilson <jim.wil...@gmail.com> wrote:

On Fri, May 27, 2022 at 6:37 AM Andrew Waterman <and...@sifive.com> wrote:

On Thu, May 26, 2022 at 11:19 PM Kito Cheng <kito....@sifive.com> wrote:

Option 2: who cares about these details

Make the CSRs always available, even when zicntr or zihpm are not
specified in the march string. Expose the test macros based on the
availability of the extension (via march string).

Pros: No compatibility issue.
Cons: Unexpected behavior (CSRs are available although the compiler
knows better); potentially harmful (compiler errors are expected to
prevent unsupported code from being generated);

The RISC-V architects have long been clear that this is the only answer that we think is sensible. (It almost seems like you are going to these mailing lists because you didn’t like the answer we provided in other forums…)

We only approved these specific backwards-incompatible changes on the basis that they will, in fact, “just work” in practice. We wouldn’t have done so for extensions that compilers actually need to target.

It is confusing for end users if the compiler handles different ISA extensions in different ways.  I think zicntr should work exactly the same as M.   If M is enabled, then the assembler accepts multiply instructions, the compiler generates multiply instructions, and the compiler defines __riscv_m so that user code can check if multiply is safe to use in an extended asm in C code or in assembly code.  If M is not enabled, then the assembler rejects multiply instructions, the compiler does not generate them, and __riscv_m is not defined so user code knows not to use multiply instructions.  zicntr should work exactly the same way.

Your argument is that because the compiler can't generate a rdcycle instruction, we don't need the same support.

Yes, that’s the extent of my argument. And I stand by it.

Not restricting the 12-bit immediate arguments of the CSRRS instruction would be a pragmatic solution.

But if we implement such a restriction correctly, then it is a one-time effort (extending would mean adding numbers to a table).

And all user benefits that Jim mentioned apply.

From a programmer's view, at least a warning would be nice if the 12-bit immediate is unexpected.

A warning could also be turned off.

[Andrew Waterman]

It should be noted that defining zicntr and zihpm is _not_ a change to any existing ratified ISA standard, despite the comments to the contrary on this thread. The counters were not part of the base ISA as it was ratified several years ago. (They were part of an earlier non-ratified version.)

The error on RVIA’s part is that we neglected to define an extension name to represent the counters. That’s what we fixed recently in defining zicntr and zihpm.

On Thu, May 26, 2022 at 11:19 PM Kito Cheng <kito....@sifive.com> wrote:

[Jim Wilson]

On Fri, May 27, 2022 at 10:03 AM Andrew Waterman <and...@sifive.com> wrote:

It should be noted that defining zicntr and zihpm is _not_ a change to any existing ratified ISA standard, despite the comments to the contrary on this thread. The counters were not part of the base ISA as it was ratified several years ago. (They were part of an earlier non-ratified version.)

The error on RVIA’s part is that we neglected to define an extension name to represent the counters. That’s what we fixed recently in defining zicntr and zihpm.

It doesn't matter how you define this.  You are still breaking compilers and other software.

Jim

 

[Kito Cheng]

The -march interface is not only used for compiler but also used for assembler,
and we also use test macros to probe if the extension is enabled or disabled,
that could also be used to check around inline asm.

Although (I assume) all existing RISC-V cores have implemented zicntr,
that seems
meaningless for now, but it's (become) an extension (the boat has
sailed, so I am
not intending to judge that), that means it's optional, so the probing becomes
meaningful.

I strongly prefer having a consistent rule to treat all extensions
instead of making exceptions if possible,
however I know that it's annoying to break the toolchain interface
immediately so that's why option 3 is there.

[Kito Cheng]

Hi Philipp:

> > ## Options 1: Ignore
> >
> > Use the same rules as other extensions and gate the access to the
> > functionality by the march strings:
> >
> > - The CSRs are only available when zicntr or zihpm are enabled.
> > - __riscv_zicntr/__riscv_zihpm are only defined when zicntr/zihpm are enabled.
> >
> > Pros: Consistent behavior with other extensions
> > Cons: Incompatible change (as discussed above)
>

> Given that these CSRs will not be widely accessed (i.e. mainly in
> software running at higher privilege modes), we might get away with
> breaking backward compatibility in this specific case.
> However, this does probably not hold for future changes that may
> impact widely visible features — so whatever solution we decide on at
> this time, should be generally applicable.

Yeah, I don't want to make exceptions if possible...having one
exception means we create a hole and might have more exceptions in
future.

> > ## Option 3: use -misa-spec to decide what to do
> >
> > Additionally to the flag -march=... the tools also have the flag
> > -misa-spec to control the behavior of the extension parser. Currently,
> > the misa-spec string can be "2.2" | "20190608" | "20191213". That flag
> > is also, where would add the new profiles ("rv20" and "rv22").
> >
> > So for the existing misa-spec strings we would keep the current
> > behavior, but for the new "rv20" and "rv22" strings, we would change
> > the behavior.
>

> From my understanding this does not entirely reflect the upstream
> specification changes: any specification version that qualifies these
> as explicitly gated by these new extension names will have to require
> the changed behavior (i.e., not just the new profiles). Consider the
> profiles as a fancy name (or a macro) expanding to a single well-known
> misa-spec= and march= combination: besides using the profiles, users
> will still be able to construct different misa-spec/march pairs and we
> will need to process them appropriately.

The purpose of adding -misa-spec=rv20|rv22 is kind of adding a spec
reference point instead of resolve the whole profile issue on the toolchain
land, Christoph just start the discussion* on toolchain SIG and that might be
better place to continue the discussion for the complete profile solution.

* https://lists.riscv.org/g/tech-toolchain-runtime/topic/91372394
(link for others who is not in the thread yet but interested for the
topic )

>
> > Pros: we don't try to solve a problem that is not tool-specific but
> > ISA-specific; we have an option to get the old functionality back (the
> > default misa-spec can be set when building a compiler using
> > "--with-isa-spec=...")
> > Cons: Users need to be made aware of the issue; more work with
> > processing error reports from users
> >
> > ## Option 4: something else?
> >
> > The three options above are not the only existing solutions. If you
> > are interested in some other solution, please share your thoughts!
> >
> > # References
> >
> > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ
> > [2] https://github.com/riscv/riscv-profiles/issues/43
>

> Thanks,
> Philipp.

[Kito Cheng]

Hi Philipp:

> I have seen that one (and it warrants a separate discussion).
> For the purpose of the discussion ongoing here, it seems important to
> clearly state that the profile is not directly tied to the ISA spec
> version (although I expect a number of subtle interactions that we'll
> need to work out, once we implement profile support).
>
> For the benefit of the wider discussion, note that the profile nowhere
> references a specific ISA version and only groups all extensions
> ratified at the date of the profile issuance into
> mandatory/optional/incompatible groups.
> Whether that means that one could combine isa-spec=2004 w/
> profile=rva22 is a different discussion and unrelated to the problem
> we are trying to solve here.

Yeah, profile is really different concept from isa-spec, and mixing
both seems not good idea,
just like I mention before my intention is want a isa-spec reference
point especially for the
zicntr and zihpm fix, but I would like it more smoothly way and
backward compatible way
like what we did for zicsr and zifencei.

So I think we could add one last new -misa-spec option, maybe 2020 or rv20, and
decoupled with the profile, ideally the default isa spec won't be changed
once we move the default to that.

[Jim Wilson]

On Wed, Jun 1, 2022 at 9:18 AM Greg Favor <gfa...@ventanamicro.com> wrote:

I'm not appreciating the need to reference a version of an ISA spec document.  Once an extension is ratified, it does not change from there on in ALL versions of ISA specs.  One should be able to look at ANY version of an ISA spec that contains a ratified version of the extension - which is documented up front in the Preface of the document.

ISA specs include some sections that are ratified, and some sections that are not.  The non-ratified sections may change, and hence we need to know which ISA version of those sections is being used.  rv32e is still draft for instance, and there are people building and shipping rv32e hardware.  rv128i is also draft.  I don't know of anyone building that currently, but if they do, it will probably go through multiple iterations before being ratified.  It is also possible that a ratified section could change in the future.  Some new computer language, some new hardware design, some new legal requirement for computers, etc and we have a need to change even a ratified section.  There should be a way to handle that, like an ISA version.

As a practical matter, having a ratified section doesn't immediately solve the software problem.  For instance, RVI is distributing Allwinner D1 Nezha boards that implement the pre-ratified semantics for the fmin/fmax instructions.  This is two and a half years after F was ratified.  Anyone that wants portable code can't use fmin/fmax if the difference between the old and new semantics would affect the result.  It will probably be another 7.5 years before we can safely assume that everyone has the new fmin/fmax implementation.  Multiply that by all of the ISA changes we have to deal with, and things get complicated pretty quickly.

Jim

[Kito Cheng]

> On Wed, Jun 1, 2022 at 9:18 AM Greg Favor <gfa...@ventanamicro.com> wrote:
> I'm not appreciating the need to reference a version of an ISA spec document. Once an extension is ratified, it does not change from there on in ALL versions of ISA specs. One should be able to look at ANY version of an ISA spec that contains a ratified version of the extension - which is documented up front in the Preface of the document.
>

> For zicntr and zihpm, it is further true that these have not changed since the draft versions in the December 2019 ISA specs (post 2019 ratifications). If by "zicntr and zihpm fix" you mean a version of the ISA specs that incorporate the proper extension names for the two sections of the Counters chapter, then just pick a recent version and that will contain what is otherwise the same version of Counters as appeared back in 2019 and in all the spec versions since then.

There are just lots of `draft` releases in the for the unpriv spec
after the end of 2019, I am not sure taking a random draft as a
reference point is the right way to go?

> Lastly, what do you mean or expect when saying that the default spec won't change once moved up to a current version? The ISA spec documents will continue to accrue new sections and chapters as new extensions are added to the architecture. In other words, what is the "default ISA spec" supposed to represent?

I don't intend to keep using -misa-spec if possible, add a few more
words to my sentence to express my intention more explicitly: "ideally

the default isa spec won't be changed once we move the default to

that, and deprecated -misa-spec option", and then just rely on the ISA
string and it's version.

[Palmer Dabbelt]

On Thu, 02 Jun 2022 10:04:12 PDT (-0700), Kito Cheng wrote:
>> On Wed, Jun 1, 2022 at 9:18 AM Greg Favor <gfa...@ventanamicro.com> wrote:
>> I'm not appreciating the need to reference a version of an ISA spec document. Once an extension is ratified, it does not change from there on in ALL versions of ISA specs. One should be able to look at ANY version of an ISA spec that contains a ratified version of the extension - which is documented up front in the Preface of the document.
>>
>> For zicntr and zihpm, it is further true that these have not changed since the draft versions in the December 2019 ISA specs (post 2019 ratifications). If by "zicntr and zihpm fix" you mean a version of the ISA specs that incorporate the proper extension names for the two sections of the Counters chapter, then just pick a recent version and that will contain what is otherwise the same version of Counters as appeared back in 2019 and in all the spec versions since then.
>
> There are just lots of `draft` releases in the for the unpriv spec
> after the end of 2019, I am not sure taking a random draft as a
> reference point is the right way to go?

I've always been very strongly against picking our own versions, that's
going to make a huge mess for everyone -- it'd be like trying to ship
GCC trunk at some arbitrary point, that'd just be chaos.

>> Lastly, what do you mean or expect when saying that the default spec won't change once moved up to a current version? The ISA spec documents will continue to accrue new sections and chapters as new extensions are added to the architecture. In other words, what is the "default ISA spec" supposed to represent?
>
> I don't intend to keep using -misa-spec if possible, add a few more
> words to my sentence to express my intention more explicitly: "ideally
> the default isa spec won't be changed once we move the default to
> that, and deprecated -misa-spec option", and then just rely on the ISA
> string and it's version.

That'd be awesome, but IMO it's just not going to work out in the real
world: we've had a decade of incompatibilities between specification
versions and there doesn't appear to be any signs that's going to stop
any time soon. I think we just have a different definition of what
compatibility is between the various groups.

Maybe I'm just out of it here, but I'm not entirely clear on how the
specs are going to be released going forwards. A lot of this sounds
like there's going to be rolling reseases, which will make sticking to
anything but the latest ISA spec tricky. Picking our own versions is a
little less scary for rolling releases, but another option would be to
just document the incompatibilities as feature flags and keep
compatibility that way.

[Christoph Müllner]

I asked this in today's SIG Toolchain call and the response was,

that it is planned to make publication books that cover all ratified extensions.

However, that is currently blocked by the transition process from LaTex to AsciiDoc.

And even for rolling releases, there are snapshots that can be uniquely identified

(similar to the current ISA specification versions).

Whatever we do here, I would prefer to avoid toolchain-specific artificial versions as

I fear that this will cause more problems than solutions.

How would you see the feature-based flag realized?

Do you mean something like "-menable-csr=all" or "-menable-csr=cntr,hpm"?

If yes, wouldn't such flags just be aliases for adding zicntr and zihpm to the march string?

I.e. what would be the benefit over "-march=+zicntr+zihpm" (a feature that we will likely need anyway)?

 

[Palmer Dabbelt]

There's a big difference between snapshots and releases, though. The
issue isn't uniquely identifying the contents, we have git for that,
it's making sure that those contents are suitable for being brought into
production software. That's not something that generally comes along
with snapshots, it requires things like review.

> Whatever we do here, I would prefer to avoid toolchain-specific artificial
> versions as
> I fear that this will cause more problems than solutions.
>
> How would you see the feature-based flag realized?
> Do you mean something like "-menable-csr=all" or "-menable-csr=cntr,hpm"?
> If yes, wouldn't such flags just be aliases for adding zicntr and zihpm to
> the march string?
> I.e. what would be the benefit over "-march=+zicntr+zihpm" (a feature that
> we will likely need anyway)?

The feature flags would be there to describe the bits we're depending on
that aren't in the latest ISA spec, whether they're just omitted or have
changed since a previous one. So it'd be something like
"-mspec-has-zifencei", "-mspec-has-speculative-pte-updates", or
"-mpause-modifies-pc". This would allow users to opt into the new
behaviors on a case-by-case basis (and then the defaults to be changed,
as we figure out what ended up implemented). That's a bit different
than the version-based approach we have right now, but it has some
benefits in that we can deprecate things that weren't implemented
without tossing the entire spec.

It's sort of just a different type of complexity: with feature flags you
can handle issues that are found long after a spec has been implemented,
as it's just the behaviors that are being selected. The flip side is
that we'll end up with another combinational explosion of targets, and
given how much of an issue we have with those already that's a bit
scary.

[Christoph Müllner]

Ok, understood.

So in the Zicntr/Zihpm case, it would not matter much, but we might have other cases as well,

where we cannot adjust the behaviour via adding/subtracting from the march string.

For sure these feature flags would be better than using -misa-spec=20191213 from a documentation perspective.

E.g. including Zicntr and Zihpm as part of the base ISA if -misa-spec=20191213 would look odd if one is not aware

of the situation discussed in this email thread. And mixing that with other mentioned behavior changes will be

hard to understand.

But coming back to the original issue in this email thread.

A backward-compatible solution would be to copy the behavior of Binutils:

Add a flags "-mcsr-check" and "-mno-csr-check" that control if CSRs should be checked.

Binutils defaults to "-mno-csr-check", but GCC could be more strict (introducing a change of behavior).

Would this be acceptable for everyone?

If not, would a flag "-menable-csr=cntr,hpm" be ok instead?

If not, what would be the preferred flag for this issue instead?

Assuming we find an agreement on the flag-naming/sematic, let's define the next steps.

Given this is a GNU-related topic, I would expect that this won't get adopted by the toolchain conventions.

Instead, this would just get implemented and documented as RISC-V target flags in GCC.

My summary of required tasks would be as follows:

* Implement Zicntr and Zihpm support

* Enable/disable all CSRs (incl. Zicntr and Zihpm) in base ISA and add/document a new flag and its behavior (depending on what we agree on)

* Add the change (and potential workaround instructions) to the upstream release notes

* All changes above will be made for GCC (and Binutils if necessary)

* No intention to backport these changes to existing release branches

Does this sound ok to everyone?

Note, that I try to push this issue forward, so we won't end up having done anything in a couple of months

because we did not manage to agree on a solution. If there is interest then I am also open to working out

a common proposal in a call.

[Christoph Müllner]

I haven't heard anything back on that, therefore I put it on the
agenda for today's Toolchain's SIG call.

There we agreed that we want -mcsr-check and -mno-csr-check flags for
GCC like in Binutils
with the same default behavior (-mn-csr-check).

Please speak up, if you prefer another solution.

BR
Christoph

[Palmer Dabbelt]

I'm not entirely sure what the proposal is: the csr-check arguments are
really there to indicate how strict binutils is about following the ISA,
that's a bit different than enabling/disabling an extension (the
attributes, for example). I don't see any issues adding
-m{no-,}csr-check to GCC so it can more easily be passed to binutils,
but I also don't see how that fixes the problem at hand.

Maybe I've lost the thread here, looking at the ratified ISA spec I
can't find Zicntr/Zihpm anywhere. Does someone have a pointer to the
spec that defines these?

[Palmer Dabbelt]

On Tue, 21 Jun 2022 18:57:36 PDT (-0700), gfa...@ventanamicro.com wrote:

> On Tue, Jun 21, 2022 at 6:50 PM Palmer Dabbelt <pal...@dabbelt.com> wrote:
>
>> Maybe I've lost the thread here, looking at the ratified ISA spec I
>> can't find Zicntr/Zihpm anywhere. Does someone have a pointer to the
>> spec that defines these?
>>
>

> Here's a summary of the current state of affairs and the intentions coming
> soon:
>
> The *latest* Unpriv spec on github shows two sections in the Counters
> chapter - titled with their respective extension names (Zicntr and Zihpm).
>
> As the table at the beginning of the Unpriv spec indicates, the Counters
> chapter is Draft but not Ratified. This will be rectified shortly, i.e,
> taking the chapter, with a couple of added clarifications, through the
> standard review and ratification process.

OK, then it sounds like we should wait until the spec is at least frozen
before adding it to the various upstream bits as presumably the
ratification process will clear up the versioning and such? None of
this releases for another few months anyway, so there should be plenty
of time to get it sorted out.

[Christoph Müllner]

Here is a quick follow-up on this topic.
We have discussed this today in the GNU RISC-V community call.

There are no objections to implementing -m{no-}csr-check in GCC
with the same default behavior as Binutils has (CSRs checks are
disabled by default).
This can start right now.

Besides that, the toolchain projects need to learn about "zicntr" and "zihpm"
and which CSRs are enabled if provided as part of the march string
(once these extensions are at least frozen):
* Zicntr: RDCYCLE(H), RDTIME(H), RDINSTRET(H)
* Zihpm: hpmcounter3–hpmcounter31
Also, we need to disable these CSRs for base (this is disputed).

There were worries that this might not be sufficient.
E.g. in the case that -mcsr-check is enabled and the arch string does
not contain
zicntr and zihpm, there comes the question of the CSRs from Zicntr and
Zihpm should
be accepted (as it is now) or not.
Given that -mcsr-check is a new flag, I (personally) don't think that
this is necessary.

[Palmer Dabbelt]

On Thu, 30 Jun 2022 09:45:31 PDT (-0700), christoph...@vrull.eu wrote:
> Here is a quick follow-up on this topic.
> We have discussed this today in the GNU RISC-V community call.
>
> There are no objections to implementing -m{no-}csr-check in GCC
> with the same default behavior as Binutils has (CSRs checks are
> disabled by default).
> This can start right now.

I guess we'd really need to see patch for sure, but there's not really a
whole lot that GCC does with CSRs so if it's just a matter of passing
this argument through from the compiler driver to the assembler then I
don't really see a lot of room for controversy. I'm not sure it's a
super important feature or anything, but we generally try to have
ISA-related arguments get passed through from gcc to gas as it's simpler
for users.

That said, I don't really think it has a whole lot to do with the
Zicntr/Zihpm issue: -mno-csr-check just ignores a big chunk of the ISA.
It's the default because earlier toolchains weren't particularly
accurate about when to accept the various CSRs and we don't want to
break compatibility.

> Besides that, the toolchain projects need to learn about "zicntr" and "zihpm"
> and which CSRs are enabled if provided as part of the march string
> (once these extensions are at least frozen):
> * Zicntr: RDCYCLE(H), RDTIME(H), RDINSTRET(H)
> * Zihpm: hpmcounter3–hpmcounter31
> Also, we need to disable these CSRs for base (this is disputed).
>
> There were worries that this might not be sufficient.
> E.g. in the case that -mcsr-check is enabled and the arch string does
> not contain
> zicntr and zihpm, there comes the question of the CSRs from Zicntr and
> Zihpm should
> be accepted (as it is now) or not.
> Given that -mcsr-check is a new flag, I (personally) don't think that
> this is necessary.

IMO that really depends on what ends up being ratified.