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

Define -march string #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cmuellner
Copy link
Collaborator

@cmuellner cmuellner commented Nov 8, 2022

This is essentially a collection of all recent change requests for the -march string:

Most of this patch is based on proposals and inputs from Kito and Palmer (see linked resources above).

It might not be perfect, but it could serve as a reasonable initial draft for further discussions.

Comments are welcome!

CC: @palmer-dabbelt @kito-cheng @asb @jrtc27

@cmuellner
Copy link
Collaborator Author

@palmer-dabbelt commented the following on the GCC mailing list:

IMO trying to handle this with another RISC-V spec is a waste of time:
we've spent many years trying to follow the specs here, it's pretty
clear they're just not meant to be read in that level of detail. This
sort of problem is all over the place in RISC-V land, moving to a
different spec doesn't fix the problem.

As I wrote in my reply message, I think -march should be documented here (as it is already) because this repo is the common place to document things like that for GCC and LLVM.

Also, I don't think a common -march documentation/specification is a waste of time. But of course, I don't consider my proposal perfect. Technical comments will help to improve it. If someone else wants to drive this issue instead, feel free to do so.

README.mkd Outdated
* The next part consists of single-letter ISA extensions, which may be in
non-canonical order.
* The next part consists of multi-letter ISA extensions, which may be in
non-canonical order.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to relax the order between single-letter / multi-letter ext, see #14

Changes:
2021/08/16: Remove order constraint between single-letter and multi-letter, also add 2 more NOTE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

README.mkd Outdated
* `rv64gc_xtheadba`: Valid `-march` string.
* `rv64gc_Zicsr`: Invalid because of the uppercase character.
* `rv32mai`: Invalid because base ISA does not follow `rv32`.
* `rv32i_zicsr_m`: Invalid because `m` after multi-letter extension.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will become valid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

README.mkd Outdated

The following interpretation differences exist:

* The expansion of `g` with versions before `20190609` is `imafd`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo (here and below): 20190609 -> 20190608

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

* `rv32i_zicsrzifencei`: Invalid because multi-letter extensions need to be
separated by an underscore (`zicsr` and `zifencei` are existing individual
extensions).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last specification defines "dependency" notion for extensions. We need to either mention that -march= respects this convention, or state that -march= doesn't follow it.
See 29.3 of 20191213: Some ISA extensions depend on the presence of other extensions, e.g., "D" depends on "F" and "F" depends on "Zicsr". These dependences may be implicit in the ISA name: for example, RV32IF is equivalent to RV32IFZicsr, and RV32ID is equivalent to RV32IFD and RV32IFDZicsr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting this.
I think it is expected by users that the march string follows this implicit enablement of extensions.
I will add a sentence to clarify this.

@eopXD
Copy link

eopXD commented Nov 22, 2022

Thank you for the work.

Trying to understand how the interpretation works so maybe I can help on the LLVM side. Is there an existing table for interpretation(s) to the ISA? If so this will help me clarify the confusion that I have to both "dates" and "version number" coexists (based on this PR - 2.2 and 20190608) as an identifier to an interpretation.

@cmuellner
Copy link
Collaborator Author

Trying to understand how the interpretation works so maybe I can help on the LLVM side. Is there an existing table for interpretation(s) to the ISA? If so this will help me clarify the confusion that I have to both "dates" and "version number" coexists (based on this PR - 2.2 and 20190608) as an identifier to an interpretation.

I tried to cover exactly this by the amount of examples included in the PR.
If there are unclear cases, then I think we should list them in this document anyway.

Regarding the ISA versions: we will just have to support all existing versioning schemes: so versions are well-ordered, but not lexicographic sortable, strings.

@cmuellner
Copy link
Collaborator Author

I've updated the PR according to the provided feedback.
Thanks for the comments!

This is essentially a collection of all recent
change requests for the -march string:

* Relax the ISA string order ([1])
* Add custom extensions ([2])
* Add profiles support ([3])

Most of this patch is based on proposals from Kito Cheng <kito.cheng@gmail.com>
(see linked resources below).

[1] riscv-non-isa#14
[2] riscv-non-isa#1
[3] https://lists.riscv.org/g/sig-toolchains/message/379
[4] riscv-non-isa#11

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
@cmuellner
Copy link
Collaborator Author

Kito mentioned that there should be more details regarding the versioning of extensions. I've added some details about that to the PR.

@jrtc27
Copy link

jrtc27 commented Dec 1, 2022

We should have -march=isa+ext for completeness (yes, _ works in that case, but then you have to care about whether it’s an ISA or profile as your “base”). We also need -march=+foo so you can add to whatever the default is (whether that’s from a flag in user-provided CFLAGS or the system default when not specified). Maybe also -march=-foo, though that gets a bit weird (but that already needs to work in the assembler due to .option arch, so give it the same semantics).

@jrtc27
Copy link

jrtc27 commented Dec 1, 2022

I also worry about what happens if a user gives CFLAGS=“-march=isa -misa-spec=…” and a build system then adds -march=profile given you’ve said it’s an error to combine those.

Copy link

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support this effort. But I think it is worth calling out explicitly where it breaks compatibility with the ISA string as defined in Vol. I Ch. 29, since existing software and build systems are following those conventions already for march. I give two examples below of compatibility breaks. These are just the first two that immediately jumped out at me (I didn't go through the whole PR carefully).

which may be in non-canonical order.
* Single-letter ISA extensions may be separated from other single-letter extensions
by an underscore (`_`).
* Multi-letter ISA extensions must be separated from other extensions
Copy link

@nick-knight nick-knight Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vol. I Ch. 29 says:

Extensions with the "Z" prefix must be separated from other multi-letter extensions by an under-
score, e.g., "RV32IMACZicsr_Zifencei".

Note there is no underscore between C and Zicsr.

Copy link
Collaborator Author

@cmuellner cmuellner Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I properly described what I wanted to express, but I was not aware that -march=rv64imafdczbb is valid in GCC and clang. I will update this to match the currently implemented behavior to reduce the differences to the ISA strings.

Fun fact: GCC currently accepts -march=rv64imafdczbbzba and clang does not.
But I don't think that's an issue.


A valid `-march` string in the ISA-string-based format must follow these rules:

* All characters of the string must be lowercase.
Copy link

@nick-knight nick-knight Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vol. I Ch. 29 says:

The ISA naming strings are case insensitive.

Indeed, the examples given therein use mixed case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is listed as the first bullet "All characters of the string must be lowercase".
So I think this is already properly documented.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now that this is already enforced by existing toolchains, so in this case I was wrong to say that

existing software and build systems are following those conventions already for march.

I still think it is worth a remark about incompatibilities with the ISA spec, but such nonnormative text can always be added later.

@asb
Copy link
Contributor

asb commented Dec 9, 2022

I do worry that profile strings might be rather cryptic and easily confusable (by humans) with ISA naming strings. If it's agreed this is problematic, it's not clear if it's best handled in the profiles spec or in how the profile names are represented in -march arguments.

i2020u32 is cryptic but perhaps less easily confusable than rvi20u32. As we get closer to 2032, rvi32u32 vs the ISA naming string rv32i... seems potentially confusing. As Andrew argues in riscv/riscv-profiles#82 it's possible that the confusability won't matter as almost nobody is using ISA naming strings.

@cmuellner
Copy link
Collaborator Author

As mentioned in the profiles ticket, would it be ok, to mention in a note that tools/compilers may impose restrictions about which march string format is accepted (e.g. via prefix or cmdln flag) and that users may look in to the compiler manual for details? So we would just define the formats here and leave some room for further restrictions for tools/compilers (if any).

@cmuellner
Copy link
Collaborator Author

We should have -march=isa+ext for completeness (yes, _ works in that case, but then you have to care about whether it’s an ISA or profile as your “base”). We also need -march=+foo so you can add to whatever the default is (whether that’s from a flag in user-provided CFLAGS or the system default when not specified). Maybe also -march=-foo, though that gets a bit weird (but that already needs to work in the assembler due to .option arch, so give it the same semantics).

I'm not sure if we should define this here (unless enough people want this). The idea is good, but it feels a bit like an added-value feature of a tool/compiler.

I also worry about what happens if a user gives CFLAGS=“-march=isa -misa-spec=…” and a build system then adds -march=profile given you’ve said it’s an error to combine those.

Same for this. I would let this behavior be defined by the tool as I fear that different tools prefer different behavior (e.g. accepting the last provided flag vs. throwing an error vs. ./configure flag to define the policy...).

I would prefer to restrict the description to a minimum that's usable by a user. Compilers and tools can then derive from here and extend more features.

Would this be ok?

@jrtc27
Copy link

jrtc27 commented Dec 9, 2022

We should have -march=isa+ext for completeness (yes, _ works in that case, but then you have to care about whether it’s an ISA or profile as your “base”). We also need -march=+foo so you can add to whatever the default is (whether that’s from a flag in user-provided CFLAGS or the system default when not specified). Maybe also -march=-foo, though that gets a bit weird (but that already needs to work in the assembler due to .option arch, so give it the same semantics).

I'm not sure if we should define this here (unless enough people want this). The idea is good, but it feels a bit like an added-value feature of a tool/compiler.

I also worry about what happens if a user gives CFLAGS=“-march=isa -misa-spec=…” and a build system then adds -march=profile given you’ve said it’s an error to combine those.

Same for this. I would let this behavior be defined by the tool as I fear that different tools prefer different behavior (e.g. accepting the last provided flag vs. throwing an error vs. ./configure flag to define the policy...).

I would prefer to restrict the description to a minimum that's usable by a user. Compilers and tools can then derive from here and extend more features.

Would this be ok?

If GCC and LLVM do different things for any of these it will be a pain point, so no, I think they really do need to be documented conventions. Otherwise either nobody will use them or they'll use them and be locked into one compiler for no good reason.

@kito-cheng
Copy link
Collaborator

Following content has send in another private mail thread, but I think it could be also post here:


I agree the naming of profile can be improved to prevent the confusion, but actually I am less concern about the confusion on the -march string in practice.

Consider the following two cases (and use the most confused possible
name rvi32u32):

  1. -march= with profile name only without extra extension
    -march=rvi32u32 - the possible and legal misspell version should
    be rv32iu32 which is rv32i base plus U extension with version 32,
    that's generally a useless configuration, should got error soon when
    trying to build/compile/assemble anything.

  2. -march= with profile name only with extra extensions
    -march=rvi32u32+xsffoo profile rvi32u32 with xsffoo vendor
    extension, and that's apparently a profile based arch string, since
    there is a plus sign there, it's only used in profile style -march
    string
    Arguable point might has 2:
    first is plus sign not included in the ISA string now, but
    used/defined in future

    • Low probability, but I guess we could...try to stop when that happened.
      second is we might also want to accept the plus sign on the
      normal ISA-string-like -march option.
    • This could be prevent when we relax the canonical order
      requirement of -march string

So based on the above two situations I think that should not be
confusing (or misuse with misspellings) in practice.

@cmuellner
Copy link
Collaborator Author

In today's Toolchain SIG call, we agreed to split this PR into multiple ones not to mix different issues and to move this forward:

  1. document current -march string
  2. document -march string relaxation
  3. add custom extensions
  4. add profiles support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants