PR for llvm/llvm-project#60889 #324
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch is related to the discussion in <https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/1>. It slightly expands the coverage of behaviour for unrecognized extensions (there are slightly different forms of error for zfoo vs e.g. 'y'), and adds coverage for an extension with an unrecognized/unsupported version number. Use "unrecognized" terminology because the expectation is the error checking will be reduced in a follow-up patch posted for review. (cherry picked from commit 8b50048)
…scv-attributes-place.s Per the psABI, the arch string should be normalised to (amongest other things) always include the full version of each extension in form zfoo1p0. riscv-attributes-place.s didn't conform to this, which is not a problem for the current parsing logic, but this behaviour would change with a patch I'm about to propose. This makes riscv-sttributes-place.s feature a valid arch string, and maintains test coverage for this particular form of invalid arch string by adding it to riscv-attributes.s. (cherry picked from commit 179a24c)
…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)
|
@MaskRay What do you think about merging this PR to the release branch? |
|
LGTM |
|
To the release manager: I apologise this is very late in the release cycle. If it's possible to merge ahead of 16.0.0 we'd like to do so, as we consider this a serious regression. You can see the problem this regression caused projects in e.g. ClangBuiltLinux/linux#1777 The change touches only RISC-V specific code, which hopefully reduces the risk of introducing it at this late stage:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves llvm/llvm-project#60889