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

Add RISC-V Profiles format #36

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

pz9115
Copy link

@pz9115 pz9115 commented May 2, 2023

This is a subset work form #26.
Add RISC-V Profiles format only. The doc record in:
https://docs.google.com/document/d/1TZXRIgVfQHWQ6xrZflHXUCSav6xNmliojrW2bEsvPno/edit.

Add RISC-V Profiles format.

Signed-off-by: Jiawei <[email protected]>
README.mkd Outdated
### Profile-based format

Profiles should be recognized and used in the `-march=` option.
The benefit is that it is easy for toolchain parsing the profiles string and expanding it into normal extensions combinations. It’s also easy to combine a profile with any extensions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the right place to be advocating for why profiles make sense.

Regarding "It's also easy to combine a profile with any extensions" - this patch should give an example of that.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in new version, remove this descriptions, give an example behind.

README.mkd Outdated
Comment on lines 81 to 82
If the 'C' (compressed) instruction set extension is targeted, the compiler
will generate compressed instructions where possible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those two lines should move before ### Profile-based format?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your metion, moved~

README.mkd Outdated
which include all the mandatory extensions required by this profile.
Here we are `not dealing with optional extensions` to profiles in the toolchain.

Moreover, some extensions are still not ratified yet. So the toolchain only keeps their name strings only.
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 prefer drop that, RVA22 ratified, and means those ext. defined in RVA22 also ratified.

[1] https://github.com/riscv/riscv-profiles/releases/tag/v1.0

Copy link
Author

Choose a reason for hiding this comment

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

Droped these two lines.

README.mkd Outdated Show resolved Hide resolved
Update profiles format descriptions, follow all comments in discuss. Thanks Alex Bradbury and Kito Cheng's review.

Signed-off-by: Jiawei <[email protected]>
@pz9115 pz9115 requested review from asb and kito-cheng June 29, 2023 04:47
README.mkd Outdated
`-march` option is easy for toolchain parsing the profiles string and expanding
it into normal extensions combinations.

Profiles format has the following form `-march=<profile-name>[+<option-ext>]+`.
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 suggest define that in BNF.

Copy link
Author

Choose a reason for hiding this comment

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

Updated, thanks for the suggestions.

@preames
Copy link

preames commented Jul 17, 2023

This got briefly brought up during the SIG toolchain meeting today, but got rapidly derailed into a process discussion. Since the root point seems important, I am repeating it here.

In addition to defining the syntax for the -march string for profiles, there is a separate set of task items required to define and implement support for each of the new extensions which were defined in the ratified profile document.

From the perspective of LLVM, my current intent is to make the support for all the various extensions a blocker for merging the -march profile parsing support. This may evolve, and I'm open to the arguments on why we should adopt a different posture, but that argument will need to be explicitly made.

@pz9115
Copy link
Author

pz9115 commented Jul 17, 2023

This got briefly brought up during the SIG toolchain meeting today, but got rapidly derailed into a process discussion. Since the root point seems important, I am repeating it here.

In addition to defining the syntax for the -march string for profiles, there is a separate set of task items required to define and implement support for each of the new extensions which were defined in the ratified profile document.

From the perspective of LLVM, my current intent is to make the support for all the various extensions a blocker for merging the -march profile parsing support. This may evolve, and I'm open to the arguments on why we should adopt a different posture, but that argument will need to be explicitly made.

You are right, Christoph added a comment here riscv-admin/dev-partners#16 (comment). In fact, we need to nothing until those new extensions are actually ratified(more details are needed in extenisons specification to approach). So I think maybe is a good idea that only implements the new extensions' names in toolchain Profiles support.

README.mkd Outdated

`-march = rv64imafdc_zicsr_ziccif_ziccrse_ziccamoa_zicclsm_za128rs_zba_zbb_zbc_zbs`

and `-march = rva20u64` is an illegal profile input, it not use uppercase letters.
Copy link
Contributor

Choose a reason for hiding this comment

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

"it not use" -> "it does not use"

Copy link
Author

Choose a reason for hiding this comment

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

Updated, thanks for your checking.

Choose a reason for hiding this comment

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

Isn't this incorrect due to the spaces around the '=' character?

-march = rv64imafdc_zicsr_ziccif_ziccrse_ziccamoa_zicclsm_za128rs_zba_zbb_zbc_zbs

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right, remove the incorrect spaces, thanks!

Update the syntax of description.

Signed-off-by: Jiawei <[email protected]>
@pz9115 pz9115 requested a review from topperc July 18, 2023 07:11
Comment on lines +92 to +95
To distinguish between ordinary extension input and input with profiles,
profiles are assumed to be entered `at the beginning of the -march option`, and
then input other extensions. Profiles `should use uppercase letters` in the `-march`
option.

Choose a reason for hiding this comment

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

Incorrect placement of code (backtick) markup?

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to emphasize here the rules for using, do you have any good solutions?

Choose a reason for hiding this comment

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

Suggested change
To distinguish between ordinary extension input and input with profiles,
profiles are assumed to be entered `at the beginning of the -march option`, and
then input other extensions. Profiles `should use uppercase letters` in the `-march`
option.
To distinguish between ordinary extension input and input with profiles,
profiles are assumed to be entered `at the beginning of the -march option`, and
then input other extensions. Profiles *should use uppercase letters* in the `-march`
option.

README.mkd Outdated
Comment on lines 97 to 107
e.g. `-march = RVA20U64` is a legal profile input, it will be expanded into:

`-march = rv64imafdc_zicsr_ziccif_ziccrse_ziccamoa_zicclsm_za128rs`,
which include all the mandatory extensions required by this profile.

`-march = RVA20U64+zba_zbb_zbc_zbs` is also a legal profile input, it will add
four new extensions after expanded profile strings:

`-march = rv64imafdc_zicsr_ziccif_ziccrse_ziccamoa_zicclsm_za128rs_zba_zbb_zbc_zbs`

and `-march = rva20u64` is an illegal profile input, it does not use uppercase letters.

Choose a reason for hiding this comment

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

Spaces around '=' character are incorrect?

Copy link
Author

Choose a reason for hiding this comment

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

Removed, thank you!

Remove incorrect spaces using.

Signed-off-by: Jiawei <[email protected]>
@pz9115 pz9115 requested a review from TommyMurphyTM1234 July 19, 2023 08:25
@cmuellner
Copy link
Collaborator

The status of the profiles support was briefly discussed in today's SIG toolchain call:

  • this was discussed at the GNU Cauldron with Kito and Palmer (I did not attend, therefore I can't say exactly what was discussed there)
  • patches are expected to get on the list this month (October 2023)

@cmuellner
Copy link
Collaborator

An initial patch was sent to the GCC mailing list today: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637490.html

@jrtc27
Copy link

jrtc27 commented Nov 21, 2023

Uppercase strings being required in things like -march is very unusual; I would be much more in favour of keeping things lowercase

@cmuellner
Copy link
Collaborator

The architecture review committee has sent out the following text about how profiles support in toolchains should look like:

In response to some recent discussion in the Apps and Tools HC about how profiles should be represented in GCC/LLVM, the ARC provides this answer: compilers should use a single parameter for an ISA string. An ISA string begins with either a base ISA name (e.g. rv64i) or a profile name (e.g. rva23u64) and is optionally followed by additional extensions (e.g. rv64imac_zicond or rva23u64_zfh_zicond). If the ISA string begins with a profile name, it is equivalent to replacing the profile name with its mandatory base ISA and its mandatory extensions; any optional extensions in a profile must be explicitly named if their inclusion is desired. ISAs are sets, and concatenating strings takes the union, so redundancy is legal (e.g. rva23u64, rva23u64_zicsr, and rva23u64_zicsr_zicsr are all valid and equivalent).

https://lists.riscv.org/g/tech-unprivileged/message/611

@wangpc-pp
Copy link

Draft LLVM implementation: llvm/llvm-project#76357.

wangpc-pp added a commit to wangpc-pp/llvm-project that referenced this pull request Dec 28, 2023
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.

We may need to pass it to backend so that we can emit an ELF attr
proposed by
riscv-non-isa/riscv-elf-psabi-doc#409.
@wangpc-pp
Copy link

Reverse ping. Is there any progress?

@cmuellner
Copy link
Collaborator

The GCC implementation is tracked here: riscv-admin/dev-partners#16.

There was a request to wait on the GCC patch regarding the adoption of profiles in toolchain components: https://gcc.gnu.org/pipermail/gcc-patches/2023-December/641025.html.

So, at this point, we have clear guidelines from RVI regarding the adoption. We have this PR, and we have patches for GCC and LLVM. This can all move forward if upstream maintainers are fine with it.

Given the relatively low impact on toolchains, I'm fine with waiting until the GCC 15 branch opens up.
LLVM might move on earlier.

wangpc-pp added a commit to wangpc-pp/llvm-project that referenced this pull request Feb 20, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
wangpc-pp added a commit to wangpc-pp/llvm-project that referenced this pull request Mar 5, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
wangpc-pp added a commit to wangpc-pp/llvm-project that referenced this pull request Mar 11, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
wangpc-pp added a commit to wangpc-pp/llvm-project that referenced this pull request Mar 18, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
wangpc-pp added a commit to llvm/llvm-project that referenced this pull request Mar 22, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
wangpc-pp added a commit to llvm/llvm-project that referenced this pull request Mar 22, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.

This is recommitted as 66f88de was reverted because of failures
caused by lacking `--target` option.
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
This PR implements the draft
riscv-non-isa/riscv-toolchain-conventions#36.

Currently, we replace specified profile in `-march` with standard
arch string.

This is recommitted as 66f88de was reverted because of failures
caused by lacking `--target` option.
@cmuellner
Copy link
Collaborator

LLVM support landed upstream a few days ago (see also llvm/llvm-project#76357).

@kepstin
Copy link

kepstin commented Oct 20, 2024

The text in this PR currently differs from the ARC decision mentioned in #36 (comment) and the implementation in LLVM in llvm/llvm-project#76357 in two significant ways:

  • Profile names are specified in lowercase, not uppercase
  • Additional extensions are specified with _ as the separator, not +

I worry that this inconsistency might cause compatibility issues.

@wangpc-pp
Copy link

@pz9115 Hi Jiawei, can you please update the RFC according to the comment?

The architecture review committee has sent out the following text about how profiles support in toolchains should look like:

In response to some recent discussion in the Apps and Tools HC about how profiles should be represented in GCC/LLVM, the ARC provides this answer: compilers should use a single parameter for an ISA string. An ISA string begins with either a base ISA name (e.g. rv64i) or a profile name (e.g. rva23u64) and is optionally followed by additional extensions (e.g. rv64imac_zicond or rva23u64_zfh_zicond). If the ISA string begins with a profile name, it is equivalent to replacing the profile name with its mandatory base ISA and its mandatory extensions; any optional extensions in a profile must be explicitly named if their inclusion is desired. ISAs are sets, and concatenating strings takes the union, so redundancy is legal (e.g. rva23u64, rva23u64_zicsr, and rva23u64_zicsr_zicsr are all valid and equivalent).

https://lists.riscv.org/g/tech-unprivileged/message/611

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.