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

Relax the ISA string order checking for -march #14

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

kito-cheng
Copy link
Collaborator

@kito-cheng kito-cheng commented Jun 22, 2021

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


It's corresponding for #11 ,

Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

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

with a minor typo fixed

README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
@ebahapo
Copy link

ebahapo commented Jul 8, 2021

It should also be specified that the tools always emit the extensions in canonical order.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Remove order constraint between single-letter
  • Add 2 NOTE
    • ELF attribute must be canonical order
    • Cross tool argument strongly recommend passed in canonical order, e.g. GCC invoke gnu assembler and LLVM with gnu assembler.

README.mkd Show resolved Hide resolved
README.mkd Show resolved Hide resolved
README.mkd Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
@kito-cheng kito-cheng force-pushed the relax-march branch 3 times, most recently from ad58d1b to 53a5bda Compare November 15, 2021 13:34
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Remove rule 2
  • Add one more rule for disambiguate version separator and 'p' ext.
  • Add note for single letter ext. with version

@lazyparser
Copy link

Hi @kito-cheng

Would you mind to update the status or progress when available?

cmuellner added a commit to cmuellner/riscv-toolchain-conventions that referenced this pull request Nov 8, 2022
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 added a commit to cmuellner/riscv-toolchain-conventions that referenced this pull request Nov 8, 2022
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 cmuellner mentioned this pull request Nov 8, 2022
cmuellner added a commit to cmuellner/riscv-toolchain-conventions that referenced this pull request Dec 1, 2022
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 added a commit to cmuellner/riscv-toolchain-conventions that referenced this pull request Dec 1, 2022
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 added a commit to cmuellner/riscv-toolchain-conventions that referenced this pull request Dec 1, 2022
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>
wangliu-iscas pushed a commit to plctlab/patchwork-binutils-gdb that referenced this pull request Dec 21, 2022
* riscv-toolchain-conventions,
PR, riscv-non-isa/riscv-toolchain-conventions#14
Issue, riscv-non-isa/riscv-toolchain-conventions#11

* Refer to the commit afc41ff,
RISC-V: Reorder the prefixed extensions which are out of order.

In the past we only allow to reorder the prefixed extensions.  But according
to the PR 14 in the riscv-toolchain-convention, we can also relax the order
checking to allow the whole extensions be written out of orders, including
the single standard extensions and the prefixed multi-letter extensions.
Just that we still need to follow the following rules as usual,

1. prefixed extensions need to be seperated with `_'.
2. prefixed extensions need complete <major>.<minor> version if set.

Please see the details in the march-ok-reorder gas testcase.

Passed the riscv-gnu-toolchain regressions.

bfd/
    * elfxx-riscv.c (enum riscv_prefix_ext_class): Changed RV_ISA_CLASS_UNKNOWN
    to RV_ISA_CLASS_SINGLE, since everything that does not belong to the
    multi-keyword will possible be a single extension for the current parser.
    (parse_config): Likewise.
    (riscv_get_prefix_class): Likewise.
    (riscv_compare_subsets): Likewise.
    (riscv_parse_std_ext): Removed, and merged with riscv_parse_prefixed_ext
    into riscv_parse_extensions.
    (riscv_parse_prefixed_ext): Likewise.
    (riscv_parse_subset): Only need to call riscv_parse_extensions to parse
    both single standard and prefixed extensions.
gas/
    * testsuite/gas/riscv/march-fail-order-std.d: Removed since the relaxed
    order checking.
    * testsuite/gas/riscv/march-fail-order-std.l: Likewise.
    * testsuite/gas/riscv/march-fail-order-x-std.d: Likewise.
    * testsuite/gas/riscv/march-fail-order-z-std.d: Likewise.
    * testsuite/gas/riscv/march-fail-order-zx-std.l: Likewise.
    * testsuite/gas/riscv/march-fail-unknown-std.l: Updated.
    * testsuite/gas/riscv/march-ok-reorder.d: New testcase.
saagarjha pushed a commit to ahjragaas/binutils-gdb that referenced this pull request Dec 23, 2022
* riscv-toolchain-conventions,
PR, riscv-non-isa/riscv-toolchain-conventions#14
Issue, riscv-non-isa/riscv-toolchain-conventions#11

* Refer to the commit afc41ff,
RISC-V: Reorder the prefixed extensions which are out of order.

In the past we only allow to reorder the prefixed extensions.  But according
to the PR 14 in the riscv-toolchain-convention, we can also relax the order
checking to allow the whole extensions be written out of orders, including
the single standard extensions and the prefixed multi-letter extensions.
Just that we still need to follow the following rules as usual,

1. prefixed extensions need to be seperated with `_'.
2. prefixed extensions need complete <major>.<minor> version if set.

Please see the details in the march-ok-reorder gas testcase.

Passed the riscv-gnu-toolchain regressions.

bfd/
    * elfxx-riscv.c (enum riscv_prefix_ext_class): Changed RV_ISA_CLASS_UNKNOWN
    to RV_ISA_CLASS_SINGLE, since everything that does not belong to the
    multi-keyword will possible be a single extension for the current parser.
    (parse_config): Likewise.
    (riscv_get_prefix_class): Likewise.
    (riscv_compare_subsets): Likewise.
    (riscv_parse_std_ext): Removed, and merged with riscv_parse_prefixed_ext
    into riscv_parse_extensions.
    (riscv_parse_prefixed_ext): Likewise.
    (riscv_parse_subset): Only need to call riscv_parse_extensions to parse
    both single standard and prefixed extensions.
gas/
    * testsuite/gas/riscv/march-fail-order-std.d: Removed since the relaxed
    order checking.
    * testsuite/gas/riscv/march-fail-order-std.l: Likewise.
    * testsuite/gas/riscv/march-fail-order-x-std.d: Likewise.
    * testsuite/gas/riscv/march-fail-order-z-std.d: Likewise.
    * testsuite/gas/riscv/march-fail-order-zx-std.l: Likewise.
    * testsuite/gas/riscv/march-fail-unknown-std.l: Updated.
    * testsuite/gas/riscv/march-ok-reorder.d: New testcase.
Copy link

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

It looks like there is a GCC patch dropping the order requirement
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642151.html

If we had fewer extensions, ensuring a canonical order is better as a
code search of a fixed string will retrieve the relevant results, and
I'd wish that we did not lose the strictness.
Now that there are a hundred extensions, and there are very bizarre use cases, I agree that enforcing a strict order has lost its goodness...

This may be similar to AArch64 where one can arbitrary specify +a+b or +b+a.

README.mkd Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator Author

GCC patch has bee merged into trunk :)

@cmuellner
Copy link
Collaborator

I think this is ready to land.

@kito-cheng kito-cheng merged commit ceb09c7 into riscv-non-isa:master Jan 30, 2024
@kito-cheng kito-cheng deleted the relax-march branch January 30, 2024 00:20
@kito-cheng
Copy link
Collaborator Author

Ohhh, we made it before 2024 lunar new year 🎉

BeMg added a commit to llvm/llvm-project that referenced this pull request Jan 30, 2024
Follow
riscv-non-isa/riscv-toolchain-conventions#14 by
dropping the order requirement of `-march`.

1. single-letter extension can be arbitrary order
    - march=rv32iamdf 
2. single-letter extension and multi-letter extension can be mixed
    - march=rv32i_zihintntl_m_a_f_d_svinval
3. multi-letter extension need seperate the following extension by
underscore, otherwise it will be intreprete as one extension.
    - march=rv32i_zbam -> i,zbam
    - march=rv32i_zba_m -> i,zba,m
topperc pushed a commit to llvm/llvm-project that referenced this pull request Jan 30, 2024
With std::move added to fix build bot failure.

Original commit message:

Follow
riscv-non-isa/riscv-toolchain-conventions#14 by
dropping the order requirement of `-march`.

1. single-letter extension can be arbitrary order
    - march=rv32iamdf
2. single-letter extension and multi-letter extension can be mixed
    - march=rv32i_zihintntl_m_a_f_d_svinval
3. multi-letter extension need seperate the following extension by
underscore, otherwise it will be intreprete as one extension.
    - march=rv32i_zbam -> i,zbam
    - march=rv32i_zba_m -> i,zba,m
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.

9 participants