[TargetVersion] Only enable on RISC-V and AArch64 #115991
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
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Piyou Chen (BeMg) ChangesAddress #115000. This patch constrains the target_version feature to work only on RISC-V and AArch64 to prevent crashes in Clang. Full diff: https://github.com/llvm/llvm-project/pull/115991.diff 2 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 509d45c0867590..6170c3c10b00ca 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3282,6 +3282,8 @@ def warn_unsupported_target_attribute
"attribute string; '%select{target|target_clones|target_version}3' "
"attribute ignored">,
InGroup<IgnoredAttributes>;
+def err_target_version_unsupported
+ : Error<"target_version attribute is not supported in this target">;
def err_attribute_unsupported
: Error<"%0 attribute is not supported on targets missing %1;"
" specify an appropriate -march= or -mcpu=">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index d05d326178e1b8..e2eaa00c666fc2 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3040,6 +3040,11 @@ bool Sema::checkTargetVersionAttr(SourceLocation LiteralLoc, Decl *D,
enum FirstParam { Unsupported };
enum SecondParam { None };
enum ThirdParam { Target, TargetClones, TargetVersion };
+
+ if (!Context.getTargetInfo().getTriple().isRISCV() &&
+ !Context.getTargetInfo().getTriple().isAArch64())
+ return Diag(LiteralLoc, diag::err_target_version_unsupported);
+
llvm::SmallVector<StringRef, 8> Features;
if (Context.getTargetInfo().getTriple().isRISCV()) {
llvm::SmallVector<StringRef, 8> AttrStrs;
|
phoebewang
approved these changes
Nov 13, 2024
Contributor
phoebewang
left a comment
phoebewang
left a comment
There was a problem hiding this comment.
We don't use it on X86, so LGTM.
Collaborator
|
Need a test |
topperc
reviewed
Nov 13, 2024
| "attribute ignored">, | ||
| InGroup<IgnoredAttributes>; | ||
| def err_target_version_unsupported | ||
| : Error<"target_version attribute is not supported in this target">; |
Collaborator
There was a problem hiding this comment.
"in this target" -> "on this target"
Contributor
Author
|
Update:
|
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
Comment on lines
3044
to
3046
| if (!Context.getTargetInfo().getTriple().isRISCV() && | ||
| !Context.getTargetInfo().getTriple().isAArch64()) | ||
| return Diag(LiteralLoc, diag::err_target_version_unsupported); |
Collaborator
There was a problem hiding this comment.
A better way to handle this is to use TargetSpecificAttr for TargetVersion in Attr.td (instead of InheritableAttr). That should automatically handle performing this check and you won't need to add a custom diagnostic.
Contributor
Author
There was a problem hiding this comment.
Great idea. This patch now uses the TargetSpecificAttr to enable target_version for AArch64 and RISC-V.
topperc
reviewed
Nov 15, 2024
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
| enum FirstParam { Unsupported }; | ||
| enum SecondParam { None }; | ||
| enum ThirdParam { Target, TargetClones, TargetVersion }; | ||
|
|
AaronBallman
approved these changes
Nov 15, 2024
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
LGTM but can you also add a release note to clang/docs/ReleaseNotes.rst letting users know about the new restriction and why.
Co-authored-by: Aaron Ballman <[email protected]>
Contributor
Author
|
Fix the merge conflict |
BeMg
added a commit
that referenced
this pull request
Nov 21, 2024
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/5536 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/5755 Here is the relevant piece of the build log for the reference |
BeMg
added a commit
to BeMg/llvm-project
that referenced
this pull request
Nov 21, 2024
…7110)" origin messenge [TargetVersion] Only enable on RISC-V and AArch64 (llvm#115991) Address llvm#115000. This patch constrains the target_version feature to work only on RISC-V and AArch64 to prevent crashes in Clang. --------- Co-authored-by: Aaron Ballman <[email protected]>
BeMg
added a commit
that referenced
this pull request
Nov 25, 2024
…" (#117128) Remain InheritableAttr to avoid the warning `TypePrinter.cpp:1953:10: warning: enumeration value ‘TargetVersion’ not handled in switch` origin messenge [TargetVersion] Only enable on RISC-V and AArch64 (#115991) Address #115000. This patch constrains the target_version feature to work only on RISC-V and AArch64 to prevent crashes in Clang. Co-authored-by: Aaron Ballman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Address #115000.
This patch constrains the target_version feature to work only on RISC-V and AArch64 to prevent crashes in Clang.