Skip to content

Conversation

@Sanjaykumar030
Copy link

This PR continues the work from #29678 to improve CPU feature detection in the Meson build system, specifically for the s390x architecture.

Problem:
The current detection for VXE and VXE2 is ambiguous:

  • VXE2: The regex 'VX.*' is overly greedy and could match any string containing “VX”.
  • VXE: The simple match for 'VX' could incorrectly enable VXE on systems that only support VX.

These ambiguities may cause NumPy to compile with unsupported CPU instructions, risking Illegal Instruction crashes or performance issues.

Solution:
This PR replaces the fragile patterns with precise regular expressions using word boundaries (\b):

  • VXE → '\\bvxe\\b'
  • VXE2 → '\\bvxe2\\b'

No changes are made to the feature hierarchy, compiler flags, or test code.

@Sanjaykumar030 Sanjaykumar030 changed the title FIX: Correct ambiguous logic for s390x CPU feature detection FIX: Correct ambiguous logic for s390x CPU feature detection Sep 6, 2025
@Sanjaykumar030
Copy link
Author

Hi @jorenham,

This PR refines the s390x CPU feature detection by replacing ambiguous VXE and VXE2 regex patterns with precise word-boundary matches. All tests pass, and there are no changes to compiler flags or feature hierarchy. Could you please review when you have a moment?

Thanks!

@charris charris merged commit 9bc8468 into numpy:main Sep 6, 2025
77 checks passed
@charris
Copy link
Member

charris commented Sep 6, 2025

Thanks @Sanjaykumar030 .

@charris
Copy link
Member

charris commented Sep 6, 2025

I'm am curious about the change to lower case. Is the match independent of case?

@charris charris added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported labels Sep 6, 2025
@charris charris changed the title FIX: Correct ambiguous logic for s390x CPU feature detection BUG: Correct ambiguous logic for s390x CPU feature detection Sep 6, 2025
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Sep 6, 2025
@Sanjaykumar030
Copy link
Author

Thanks for the question, @charris. The new regex patterns are case-sensitive, chosen to align with the lowercase feature strings reported on s390x. This keeps the behavior consistent with the existing build logic and avoids any breaking changes.

@jorenham
Copy link
Member

jorenham commented Sep 7, 2025

@charris You might want to have a look at this PR by the same author.

@seberg
Copy link
Member

seberg commented Sep 17, 2025

Just went through my emails, @ngoldbaum can you have a brief look at this? I don't know how this matching works, but given Joren's comment, it is unclear to me that the author does.

@Sanjaykumar030
Copy link
Author

Great if any other maintainer is double-checking for safety. For clarity, here’s a comparison of old vs new behavior:

CPU info: vx
OLD -> VX=True, VXE=False, VXE2=False
NEW -> VX=True, VXE=False, VXE2=False

CPU info: vxe
OLD -> VX=True, VXE=True, VXE2=False
NEW -> VX=False, VXE=True, VXE2=False

CPU info: vxenabled
OLD -> VX=True
NEW -> no match

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants