Skip to content

fix armv6kz LDREX definition #122965

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

Merged
merged 2 commits into from
Jan 15, 2025
Merged

fix armv6kz LDREX definition #122965

merged 2 commits into from
Jan 15, 2025

Conversation

Un1q32
Copy link
Contributor

@Un1q32 Un1q32 commented Jan 14, 2025

Fixes #37901

This behavior is consistent with GCC

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:ARM clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 14, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 14, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-arm

Author: None (Un1q32)

Changes

Fixes #37901

This behavior is consistent with GCC


Full diff: https://github.com/llvm/llvm-project/pull/122965.diff

1 Files Affected:

  • (modified) clang/lib/Basic/Targets/ARM.cpp (+2-1)
diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp
index 61ee26d8863832..0fd5433a76402e 100644
--- a/clang/lib/Basic/Targets/ARM.cpp
+++ b/clang/lib/Basic/Targets/ARM.cpp
@@ -617,7 +617,8 @@ bool ARMTargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
   case 6:
     if (ArchProfile == llvm::ARM::ProfileKind::M)
       LDREX = 0;
-    else if (ArchKind == llvm::ARM::ArchKind::ARMV6K)
+    else if (ArchKind == llvm::ARM::ArchKind::ARMV6K ||
+             ArchKind == llvm::ARM::ArchKind::ARMV6KZ)
       LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
     else
       LDREX = LDREX_W;

@efriedma-quic
Copy link
Collaborator

Please add a corresponding CHECK line to clang/test/Preprocessor/arm-acle-6.4.c

@davemgreen davemgreen requested a review from stuij January 14, 2025 22:03
@davemgreen
Copy link
Collaborator

Can you add a test?

@Un1q32
Copy link
Contributor Author

Un1q32 commented Jan 14, 2025

Can you add a test?

Is the CHECK line enough or do you want something more? My understanding is that the CHECK lines are tests, but I'm not sure since this was sent after the request for the CHECK line.

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

Yes, the CHECK is enough.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks LGTM. Let us know if we should squash and merge (I never know who has access).

@Un1q32
Copy link
Contributor Author

Un1q32 commented Jan 15, 2025

Yeah I don't have access, please merge thank you

@davemgreen davemgreen merged commit e00d1dd into llvm:main Jan 15, 2025
8 checks passed
@Un1q32 Un1q32 deleted the armv6kz-fix branch January 15, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:ARM clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When compiling for arch armv6kz, __ARM_FEATURE_LDREX predefined incorrectly
4 participants