Skip to content
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

Incorrect ACLE #define __ARM_FEATURE_LDREX value and __builtin_arm_ldrex(i64*) for M-profile #615

Open
piotrrak opened this issue Dec 23, 2024 · 2 comments

Comments

@piotrrak
Copy link

Hello

clang/clang++ claims incorrectly support of LDREXD/STREXD which are unsupported for M-profile.
Eg. cortex-m85 (v8.1m-main) STREX/LDREX supported variants
Moreover it will generate code for those instructions and won't reject builtins: __builtin_arm_ldrex for 64b integer types.
It goes all the way and also generates invalid code for those.

$ echo |LLVM-ET-Arm-19.1.1-Linux-x86_64/bin/clang-19 --target=thumbv8.1m.main-unknown-none-eabihf -dM -E - |grep LDREX
#define __ARM_FEATURE_LDREX 0xf

0x8 = 1u<<3 claims support dword ldrex/strex according to ACLE docs.
Issue was partially fixed for __atomic_* but not for:

  • __builtin_arm_ldrex
  • __builtin_arm_strex
  • __builtin_arm_ldaex
  • __builtin_arm_stlex

Please see llvm/llvm-project@aecdf15#diff-9e62f39a84effbc02335327401af81a94a17df39752973987b102676ed207bb1R240

gcc is (I believe) correct about this one.

echo | gcc -march=armv8.1-m.main -dM -E -|grep LDREX
#define __ARM_FEATURE_LDREX 7

Please see code below that currently is incorrectly accepted, emits incorrect @llvm.arm.ldrexd llvm intrin, which is later incorrectly lowered to invalid machine instructions for M-profile

#if !(__ARM_ARCH >= 7 && __ARM_ARCH_PROFILE == 'M')
#error "Compile eg. -std=c++23 --target=armv8.1m-unknown-none-eabihf"
#error "Compile eg. -std=c++23 --target=armv8m-unknown-none-eabihf"
#error "Compile eg. -std=c++23 --target=armv7m-unknown-none-eabihf"
#endif

#if !defined(__ARM_FEATURE_LDREX)
#error "No ACLE defines?"
#endif


using i64 = _BitInt(64);
using u64 = unsigned _BitInt(64);
using ll = long long;
using ull = unsigned long long;

static_assert(sizeof(ll) == 8);
static_assert(sizeof(ull) == 8);

// That's cool:
static_assert(not __atomic_always_lock_free(8, 0));

// Incorrectly stated true
consteval bool LIE(bool cond) { return not cond; }

// See https://arm-software.github.io/acle/main/acle.html#ldrexstrex
static_assert( LIE((__ARM_FEATURE_LDREX & 0x8) == 0)); // LIE


// Should be rejected with 64b type

static_assert( LIE(not requires (ll* p) {__builtin_arm_ldrex(p); }));
static_assert( LIE(not requires (ll* p) {__builtin_arm_ldaex(p); }));

static_assert( LIE(not requires (ull* p) {__builtin_arm_ldrex(p); }));
static_assert( LIE(not requires (ull* p) {__builtin_arm_ldaex(p); }));

static_assert( LIE(not requires (i64* p) {__builtin_arm_ldrex(p); }));
static_assert( LIE(not requires (i64* p) {__builtin_arm_ldaex(p); }));

static_assert( LIE(not requires (u64* p) {__builtin_arm_ldrex(p); }));
static_assert( LIE(not requires (u64* p) {__builtin_arm_ldaex(p); }));

// Store Register Exclusive.

static_assert( LIE(not requires (ll* p, ll v) {__builtin_arm_strex(v, p); }));
static_assert( LIE(not requires (ll* p, ll v) {__builtin_arm_stlex(v, p); }));
static_assert( LIE(not requires (ull* p, ull v) {__builtin_arm_strex(v, p); }));
static_assert( LIE(not requires (ull* p, ull v) {__builtin_arm_stlex(v, p); }));
static_assert( LIE(not requires (i64* p, i64 v) {__builtin_arm_strex(v, p); }));
static_assert( LIE(not requires (i64* p, i64 v) {__builtin_arm_stlex(v, p); }));
static_assert( LIE(not requires (u64* p, u64 v) {__builtin_arm_strex(v, p); }));
static_assert( LIE(not requires (u64* p, u64 v) {__builtin_arm_stlex(v, p); }));

// https://developer.arm.com/documentation/dui0489/i/arm-and-thumb-instructions/ldrex?lang=en

// emits ARMv6k ldrexd
auto ldrexd_u64(u64 * p) {return __builtin_arm_ldrex(p);}
auto ldrexd_i64(i64 * p) {return __builtin_arm_ldrex(p);}
auto ldrexd_ll(long long * p) {return __builtin_arm_ldrex(p);}
auto ldrexd_ull(unsigned long long * p) {return __builtin_arm_ldrex(p);}

// emits ARMv7a? ldaexd
auto ldaexd_u64(u64 * p) {return __builtin_arm_ldaex(p);}
auto ldaexd_i64(i64 * p) {return __builtin_arm_ldaex(p);}
auto ldaexd_ll(long long * p) {return __builtin_arm_ldaex(p);}
auto ldaexd_ull(unsigned long long * p) {return __builtin_arm_ldaex(p);}

Disassembly of '-Os':

builtin_ldrex.o:	file format elf32-littlearm

Disassembly of section .text:

00000000 <_Z10ldrexd_u64PDU64_>:
       0: e8d0 017f    	<unknown>
       4: 4770         	bx	lr

00000006 <_Z10ldrexd_i64PDB64_>:
       6: e8d0 017f    	<unknown>
       a: 4770         	bx	lr

0000000c <_Z9ldrexd_llPx>:
       c: e8d0 017f    	<unknown>
      10: 4770         	bx	lr

00000012 <_Z10ldrexd_ullPy>:
      12: e8d0 017f    	<unknown>
      16: 4770         	bx	lr

00000018 <_Z10ldaexd_u64PDU64_>:
      18: e8d0 01ff    	<unknown>
      1c: 4770         	bx	lr

0000001e <_Z10ldaexd_i64PDB64_>:
      1e: e8d0 01ff    	<unknown>
      22: 4770         	bx	lr

00000024 <_Z9ldaexd_llPx>:
      24: e8d0 01ff    	<unknown>
      28: 4770         	bx	lr

0000002a <_Z10ldaexd_ullPy>:
      2a: e8d0 01ff    	<unknown>
      2e: 4770         	bx	lr

I did not check clrex, but most likely also the case.

Please let me know if I it'd be better to report it upstream.
It is also reproducible with upstream clang-20 HEAD.
Just thought since it is ACLE related I will report it here.
And since it's more likely to hit LLVM-embedded-toolchain-for-Arm users, than upstream clang.

Cheers,
/P

@smithp35
Copy link
Contributor

smithp35 commented Jan 6, 2025

Thanks for the detailed report. Apologies for the delay in responding, we're just coming back off vacation.

It would be best to report this in upstream llvm-project. As a rule of thumb if there is an issue that is specific to the toolchain (integration between clang, binutils, libraries) then it is best to report it here, but if it is purely a code-generation problem with upstream clang/llvm then it is best to report that in source.

If you're OK with raising an issue in llvm-project yourself, we'd be grateful if you could do that? If not, we can raise one for you citing this.

@piotrrak
Copy link
Author

piotrrak commented Jan 7, 2025

Thank you, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants