-
Notifications
You must be signed in to change notification settings - Fork 588
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 naming rule of extension name #718
base: latex
Are you sure you want to change the base?
Conversation
This renders the parsing of Zfoo1p0 ambiguous |
How about if we require the trailing alphabetical character must not be "p"? EDIT: oops, I see @jrtc27 already pointed this out riscv/riscv-v-spec#729 (comment) |
8e0cc05
to
2b456f0
Compare
Changes:
|
There is a zbp extension from bit manipulation, although it's not included in the ratification package this time, so I add a complicated rule for that :p and let @aswaterman and @kasanovic to make the final decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor grammatical errors :)
Current spec only allow alphabetical name, which means we can't use number in the name, but we already defined serveral extension name with number, like `zve32x`, `zvl128b`.
- The second letter from the bottom cannot be a numeric if the last letter is ``p'' - For example, Zfoo1p0 is ambiguou without this extra rule: Zfoo1p extension with version 0 or Zfoo extension with version 1.0.
2b456f0
to
311ea1f
Compare
@eopXD Thanks :) |
Changes:
|
Note that #688 also changes this same extension naming section. |
RISC-V spec only allow alphabetical name for extension before, however vector extension add several extension named with digits, so we try to extend the naming rule. Ref: riscv/riscv-isa-manual#718 gcc/ChangeLog: * common/config/riscv/riscv-common.c (riscv_subset_list::parse_multiletter_ext): Allow ext. name has digit.
RISC-V spec only allow alphabetical name for extension before, however vector extension add several extension named with digits, so we try to extend the naming rule. Ref: riscv/riscv-isa-manual#718 gcc/ChangeLog: * common/config/riscv/riscv-common.c (riscv_subset_list::parse_multiletter_ext): Allow ext. name has digit.
…s/versions in RISC-V attributes This patch follows on from this RFC thread <https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/> and the ensuing discussion in the RISC-V LLVM sync-up call. The consensus agreed that the behaviour change in LLD introduced in D138550 that results in object files including arch attributes with unrecognised extensions or versions of extensions is a regression and should be treated as such. As it stands, this logic means that LLD will error out if trying to link a RISC-V object file from LLVM HEAD (rv32i2p0/rv64i2p0) with one from current GCC (rv32i2p1/rv64i2p1 by default). There's a bigger discussion about exactly when to warn vs error and so on, and how to control that, and this patch doesn't attempt to address all those questions. It simply tries to fix the problem with a minimally invasive change, intended to be cherry-picked for 16.0.x (ideally 16.0.0, but queued for 16.0.1 if we're too late in the release cycle). As you can see from the test changes, although the changed logic is mostly more permissive, it will reject some embedded arch strings that were accepted before. Because the same logic was previously used for parsing human-written -march as for the arch attribute (intended to be stored in normalised form), various short-hand formats were previously accepted. Maintaining support for such ill-formed attributes would substantially complicate the logic, and given the previous implementation was so much stricter in every other way, was surely a bug rather than a design choice. Surprisingly, the precise rules for how numbers can be embedded into extension names isn't fully defined (there is a PR to the ISA manual that is not yet merged <riscv/riscv-isa-manual#718>). In the absence of specific criteria for rejecting extensions names that would be ambiguous in abbreviated form, `RISCVISAInfo::parseArchStringNormalized` just pulls out the version information from the end of each extension description. Differential Revision: https://reviews.llvm.org/D144353
…s/versions in RISC-V attributes This patch follows on from this RFC thread <https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/> and the ensuing discussion in the RISC-V LLVM sync-up call. The consensus agreed that the behaviour change in LLD introduced in D138550 that results in object files including arch attributes with unrecognised extensions or versions of extensions is a regression and should be treated as such. As it stands, this logic means that LLD will error out if trying to link a RISC-V object file from LLVM HEAD (rv32i2p0/rv64i2p0) with one from current GCC (rv32i2p1/rv64i2p1 by default). There's a bigger discussion about exactly when to warn vs error and so on, and how to control that, and this patch doesn't attempt to address all those questions. It simply tries to fix the problem with a minimally invasive change, intended to be cherry-picked for 16.0.x (ideally 16.0.0, but queued for 16.0.1 if we're too late in the release cycle). As you can see from the test changes, although the changed logic is mostly more permissive, it will reject some embedded arch strings that were accepted before. Because the same logic was previously used for parsing human-written -march as for the arch attribute (intended to be stored in normalised form), various short-hand formats were previously accepted. Maintaining support for such ill-formed attributes would substantially complicate the logic, and given the previous implementation was so much stricter in every other way, was surely a bug rather than a design choice. Surprisingly, the precise rules for how numbers can be embedded into extension names isn't fully defined (there is a PR to the ISA manual that is not yet merged <riscv/riscv-isa-manual#718>). In the absence of specific criteria for rejecting extensions names that would be ambiguous in abbreviated form, `RISCVISAInfo::parseArchStringNormalized` just pulls out the version information from the end of each extension description. Differential Revision: https://reviews.llvm.org/D144353 (cherry picked from commit ff93c9b)
…s/versions in RISC-V attributes This patch follows on from this RFC thread <https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/> and the ensuing discussion in the RISC-V LLVM sync-up call. The consensus agreed that the behaviour change in LLD introduced in D138550 that results in object files including arch attributes with unrecognised extensions or versions of extensions is a regression and should be treated as such. As it stands, this logic means that LLD will error out if trying to link a RISC-V object file from LLVM HEAD (rv32i2p0/rv64i2p0) with one from current GCC (rv32i2p1/rv64i2p1 by default). There's a bigger discussion about exactly when to warn vs error and so on, and how to control that, and this patch doesn't attempt to address all those questions. It simply tries to fix the problem with a minimally invasive change, intended to be cherry-picked for 16.0.x (ideally 16.0.0, but queued for 16.0.1 if we're too late in the release cycle). As you can see from the test changes, although the changed logic is mostly more permissive, it will reject some embedded arch strings that were accepted before. Because the same logic was previously used for parsing human-written -march as for the arch attribute (intended to be stored in normalised form), various short-hand formats were previously accepted. Maintaining support for such ill-formed attributes would substantially complicate the logic, and given the previous implementation was so much stricter in every other way, was surely a bug rather than a design choice. Surprisingly, the precise rules for how numbers can be embedded into extension names isn't fully defined (there is a PR to the ISA manual that is not yet merged <riscv/riscv-isa-manual#718>). In the absence of specific criteria for rejecting extensions names that would be ambiguous in abbreviated form, `RISCVISAInfo::parseArchStringNormalized` just pulls out the version information from the end of each extension description. Differential Revision: https://reviews.llvm.org/D144353 (cherry picked from commit ff93c9b)
…s/versions in RISC-V attributes This patch follows on from this RFC thread <https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/> and the ensuing discussion in the RISC-V LLVM sync-up call. The consensus agreed that the behaviour change in LLD introduced in D138550 that results in object files including arch attributes with unrecognised extensions or versions of extensions is a regression and should be treated as such. As it stands, this logic means that LLD will error out if trying to link a RISC-V object file from LLVM HEAD (rv32i2p0/rv64i2p0) with one from current GCC (rv32i2p1/rv64i2p1 by default). There's a bigger discussion about exactly when to warn vs error and so on, and how to control that, and this patch doesn't attempt to address all those questions. It simply tries to fix the problem with a minimally invasive change, intended to be cherry-picked for 16.0.x (ideally 16.0.0, but queued for 16.0.1 if we're too late in the release cycle). As you can see from the test changes, although the changed logic is mostly more permissive, it will reject some embedded arch strings that were accepted before. Because the same logic was previously used for parsing human-written -march as for the arch attribute (intended to be stored in normalised form), various short-hand formats were previously accepted. Maintaining support for such ill-formed attributes would substantially complicate the logic, and given the previous implementation was so much stricter in every other way, was surely a bug rather than a design choice. Surprisingly, the precise rules for how numbers can be embedded into extension names isn't fully defined (there is a PR to the ISA manual that is not yet merged <riscv/riscv-isa-manual#718>). In the absence of specific criteria for rejecting extensions names that would be ambiguous in abbreviated form, `RISCVISAInfo::parseArchStringNormalized` just pulls out the version information from the end of each extension description. Differential Revision: https://reviews.llvm.org/D144353
RISC-V spec only allow alphabetical name for extension before, however vector extension add several extension named with digits, so we try to extend the naming rule. Ref: riscv/riscv-isa-manual#718 gcc/ChangeLog: * common/config/riscv/riscv-common.c (riscv_subset_list::parse_multiletter_ext): Allow ext. name has digit.
RISC-V spec only allow alphabetical name for extension before, however vector extension add several extension named with digits, so we try to extend the naming rule. Ref: riscv/riscv-isa-manual#718 gcc/ChangeLog: * common/config/riscv/riscv-common.c (riscv_subset_list::parse_multiletter_ext): Allow ext. name has digit.
Current spec only allow alphabetical name, which means we can't use number in
the name, but we already defined serveral extension name with number,
like
zve32x
,zvl128b
.So this PR try to make the rule up to date and add one more rule to restrict the name must end with alphabetical to prevent ambiguous with version number.