Skip to content

[Clang][ARM] Fix __ARM_FEATURE_LDREX on Armv8-M #149538

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Clang][ARM] Fix __ARM_FEATURE_LDREX on Armv8-M #149538

wants to merge 1 commit into from
+34 −1

Conversation

statham-arm
Copy link
Collaborator

The Armv8-M architecture doesn't have the LDREXD and STREXD instructions, for exclusive load/store of a 64-bit quantity split across two registers. But the __ARM_FEATURE_LDREX macro was set to a value that claims it does, because the case for Armv8 was missing a check for M profile.

The Armv7 case got it right, so I've just made the two cases the same.

@statham-arm
[Clang][ARM] Fix __ARM_FEATURE_LDREX on Armv8-M
9eb51f1 
The Armv8-M architecture doesn't have the LDREXD and STREXD
instructions, for exclusive load/store of a 64-bit quantity split
across two registers. But the `__ARM_FEATURE_LDREX` macro was set to
a value that claims it does, because the case for Armv8 was missing a
check for M profile.

The Armv7 case got it right, so I've just made the two cases the same.
@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 Jul 18, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2025

@llvm/pr-subscribers-backend-arm

Author: Simon Tatham (statham-arm)

Changes

The Armv8-M architecture doesn't have the LDREXD and STREXD instructions, for exclusive load/store of a 64-bit quantity split across two registers. But the __ARM_FEATURE_LDREX macro was set to a value that claims it does, because the case for Armv8 was missing a check for M profile.

The Armv7 case got it right, so I've just made the two cases the same.

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

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/ARM.cpp (+3-1)
  • (modified) clang/test/Preprocessor/arm-acle-6.4.c (+31)
diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp
index 7ff8e51f8a7f8..29de34bbc4fe4 100644
--- a/clang/lib/Basic/Targets/ARM.cpp
+++ b/clang/lib/Basic/Targets/ARM.cpp
@@ -623,13 +623,15 @@ bool ARMTargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
       LDREX = LDREX_W;
     break;
   case 7:
+  case 8:
     if (ArchProfile == llvm::ARM::ProfileKind::M)
       LDREX = LDREX_W | LDREX_H | LDREX_B;
     else
       LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
     break;
-  case 8:
   case 9:
+    assert(ArchProfile != llvm::ARM::ProfileKind::M &&
+           "No Armv9-M architectures defined");
     LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
   }
 
diff --git a/clang/test/Preprocessor/arm-acle-6.4.c b/clang/test/Preprocessor/arm-acle-6.4.c
index 2c8f4868263a6..48deba74c8ab2 100644
--- a/clang/test/Preprocessor/arm-acle-6.4.c
+++ b/clang/test/Preprocessor/arm-acle-6.4.c
@@ -188,6 +188,37 @@
 // RUN: %clang --target=arm-arm-none-eabi -mcpu=cortex-m33 -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-M-DSP
 // RUN: %clang --target=arm-arm-none-eabi -march=armv8m.main+dsp -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-M-DSP
 
+// RUN: %clang -target arm-none-linux-eabi -march=armv8m.base -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-BASE
+
+// CHECK-V8M-BASE-NOT: __ARM_ARCH_ISA_ARM
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_DSP
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_SIMD32
+// CHECK-V8M-BASE:     __ARM_ARCH 8
+// CHECK-V8M-BASE:     __ARM_ARCH_ISA_THUMB 1
+// CHECK-V8M-BASE:     __ARM_ARCH_PROFILE 'M'
+// CHECK-V8M-BASE:     __ARM_FEATURE_CLZ 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_IDIV 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_LDREX 0x7
+// CHECK-V8M-BASE:     __ARM_FEATURE_QBIT 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_SAT
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_UNALIGNED
+
+// RUN: %clang -target arm-none-linux-eabi -march=armv8m.main -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-MAIN
+// RUN: %clang -target arm-none-linux-eabi -march=armv8.1m.main -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-MAIN
+
+// CHECK-V8M-MAIN-NOT: __ARM_ARCH_ISA_ARM
+// CHECK-V8M-MAIN-NOT: __ARM_FEATURE_DSP
+// CHECK-V8M-MAIN-NOT: __ARM_FEATURE_SIMD32
+// CHECK-V8M-MAIN:     __ARM_ARCH 8
+// CHECK-V8M-MAIN:     __ARM_ARCH_ISA_THUMB 2
+// CHECK-V8M-MAIN:     __ARM_ARCH_PROFILE 'M'
+// CHECK-V8M-MAIN:     __ARM_FEATURE_CLZ 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_IDIV 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_LDREX 0x7
+// CHECK-V8M-MAIN:     __ARM_FEATURE_QBIT 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_SAT 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_UNALIGNED 1
+
 // CHECK-M-DSP: __ARM_FEATURE_DSP 1
 // CHECK-M-DSP: __ARM_FEATURE_SIMD32 1

@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2025

@llvm/pr-subscribers-clang

Author: Simon Tatham (statham-arm)

Changes

The Armv8-M architecture doesn't have the LDREXD and STREXD instructions, for exclusive load/store of a 64-bit quantity split across two registers. But the __ARM_FEATURE_LDREX macro was set to a value that claims it does, because the case for Armv8 was missing a check for M profile.

The Armv7 case got it right, so I've just made the two cases the same.

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

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/ARM.cpp (+3-1)
  • (modified) clang/test/Preprocessor/arm-acle-6.4.c (+31)
diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp
index 7ff8e51f8a7f8..29de34bbc4fe4 100644
--- a/clang/lib/Basic/Targets/ARM.cpp
+++ b/clang/lib/Basic/Targets/ARM.cpp
@@ -623,13 +623,15 @@ bool ARMTargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
       LDREX = LDREX_W;
     break;
   case 7:
+  case 8:
     if (ArchProfile == llvm::ARM::ProfileKind::M)
       LDREX = LDREX_W | LDREX_H | LDREX_B;
     else
       LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
     break;
-  case 8:
   case 9:
+    assert(ArchProfile != llvm::ARM::ProfileKind::M &&
+           "No Armv9-M architectures defined");
     LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
   }
 
diff --git a/clang/test/Preprocessor/arm-acle-6.4.c b/clang/test/Preprocessor/arm-acle-6.4.c
index 2c8f4868263a6..48deba74c8ab2 100644
--- a/clang/test/Preprocessor/arm-acle-6.4.c
+++ b/clang/test/Preprocessor/arm-acle-6.4.c
@@ -188,6 +188,37 @@
 // RUN: %clang --target=arm-arm-none-eabi -mcpu=cortex-m33 -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-M-DSP
 // RUN: %clang --target=arm-arm-none-eabi -march=armv8m.main+dsp -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-M-DSP
 
+// RUN: %clang -target arm-none-linux-eabi -march=armv8m.base -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-BASE
+
+// CHECK-V8M-BASE-NOT: __ARM_ARCH_ISA_ARM
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_DSP
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_SIMD32
+// CHECK-V8M-BASE:     __ARM_ARCH 8
+// CHECK-V8M-BASE:     __ARM_ARCH_ISA_THUMB 1
+// CHECK-V8M-BASE:     __ARM_ARCH_PROFILE 'M'
+// CHECK-V8M-BASE:     __ARM_FEATURE_CLZ 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_IDIV 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_LDREX 0x7
+// CHECK-V8M-BASE:     __ARM_FEATURE_QBIT 1
+// CHECK-V8M-BASE:     __ARM_FEATURE_SAT
+// CHECK-V8M-BASE-NOT: __ARM_FEATURE_UNALIGNED
+
+// RUN: %clang -target arm-none-linux-eabi -march=armv8m.main -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-MAIN
+// RUN: %clang -target arm-none-linux-eabi -march=armv8.1m.main -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-V8M-MAIN
+
+// CHECK-V8M-MAIN-NOT: __ARM_ARCH_ISA_ARM
+// CHECK-V8M-MAIN-NOT: __ARM_FEATURE_DSP
+// CHECK-V8M-MAIN-NOT: __ARM_FEATURE_SIMD32
+// CHECK-V8M-MAIN:     __ARM_ARCH 8
+// CHECK-V8M-MAIN:     __ARM_ARCH_ISA_THUMB 2
+// CHECK-V8M-MAIN:     __ARM_ARCH_PROFILE 'M'
+// CHECK-V8M-MAIN:     __ARM_FEATURE_CLZ 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_IDIV 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_LDREX 0x7
+// CHECK-V8M-MAIN:     __ARM_FEATURE_QBIT 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_SAT 1
+// CHECK-V8M-MAIN:     __ARM_FEATURE_UNALIGNED 1
+
 // CHECK-M-DSP: __ARM_FEATURE_DSP 1
 // CHECK-M-DSP: __ARM_FEATURE_SIMD32 1

erichkeane
erichkeane approved these changes Jul 18, 2025
View reviewed changes
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

I'm find with this, but am not the one who should be reviewing for correctness :) I presume the submitter knows the ARM features list better than I, so shrug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vhscampos vhscampos vhscampos approved these changes

@erichkeane erichkeane erichkeane approved these changes

@fhahn fhahn Awaiting requested review from fhahn

@TNorthover TNorthover Awaiting requested review from TNorthover

@smithp35 smithp35 Awaiting requested review from smithp35

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@statham-arm @llvmbot @vhscampos @erichkeane