Skip to content

Refactor arm detection#1272

Merged
JohanMabille merged 13 commits intoxtensor-stack:masterfrom
AntoinePrv:arm-detection-new
Apr 3, 2026
Merged

Refactor arm detection#1272
JohanMabille merged 13 commits intoxtensor-stack:masterfrom
AntoinePrv:arm-detection-new

Conversation

@AntoinePrv
Copy link
Copy Markdown
Contributor

Follow up to #1251 with similar API.
For this we need to use getauxval on Linux (no current alternative for other OS but they may come in the future).

Seems like we will be able to reuse it for RISC-V.

@AntoinePrv

This comment was marked as outdated.

@AntoinePrv AntoinePrv force-pushed the arm-detection-new branch 3 times, most recently from 8cdfb91 to 2b80376 Compare March 23, 2026 11:31
@AntoinePrv
Copy link
Copy Markdown
Contributor Author

@serge-sans-paille this is ready

* Set to 1 if NEON is available at compile-time, to 0 otherwise.
*/
#if (defined(__ARM_NEON) && __ARM_ARCH >= 7) || XSIMD_WITH_NEON64
#if (defined(__ARM_ARCH) && (__ARM_ARCH >= 7) && defined(__ARM_NEON)) || XSIMD_TARGET_ARM64
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you add this __ARM_ARCH check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought that the following __ARM_ARCH >= 7 was unsound if __ARM_ARCH was not defined.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that @serge-sans-paille is saying that if __ARM_NEON is defined, then __ARM_ARCH will also be defined as well. So defined(__ARM_NEON) && __ARM_ARCH >= 7 is already correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ha sure, any ways is fine, though this does not hurt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's revert to the previous less verbose condition.


namespace detail
{
#if XSIMD_WITH_LINUX_GETAUXVAL
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you could restrict the #if to the function body, using the (void) type trick to fake usage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, although that is the opposite of what we ended up doing for cpuid :)


inline static linux_auxval read(getauxval_t type) noexcept
{
return linux_auxval(detail::linux_getauxval(type));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the extra functional cast should not be needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ha no this is an (explicit) constructor call getauxval_t -> linux_auxval (current type).

{
using linux_getauxval_t = unsigned long;

inline linux_getauxval_t linux_getauxval(linux_getauxval_t type) noexcept;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you need to forward declare this here and implement it in the bottom.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is used on line 54 (middle of the file).
I like the bottom of the file definition as much as possible when it is full of #if etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Moving the implementation of the class after its definition would allow to remove this forward declaration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We also need using linux_getauxval_t = unsigned long; to declare the class.
Anyways this is pretty minor and I don't care exactly how this is implemented, but I don't think this is worth arguing about.

@JohanMabille JohanMabille merged commit 40256dc into xtensor-stack:master Apr 3, 2026
71 checks passed
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

Successfully merging this pull request may close these issues.

4 participants