-
Notifications
You must be signed in to change notification settings - Fork 242
fixes for 10.47 #819
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
fixes for 10.47 #819
Conversation
Since abc2ae6 (Update AX_PTHREAD (PCRE2Project#694), 2025-02-12), when using autoconf 2.60 with autogen, shows: error: possibly undefined macro: AS_ECHO Use 2.62 instead, which include that macro, as the new required version.
The current implementation needs SSE2 support (which is part of the base amd64 specification and available in most CPUs from this century. Without this change, SIMD will be only enabled if SSE4.1 is detected which is not true for older AMD cpus, even if 64bit capable. While at it, update related documentation and other minor tweaks.
zherczeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| /* The AVX2 code path is currently disabled. | ||
| #define JIT_HAS_FAST_REQUESTED_CHAR_SIMD (sljit_has_cpu_feature(SLJIT_HAS_SIMD)) | ||
| */ | ||
| #if defined(SLJIT_CONFIG_X86_64) && SLJIT_CONFIG_X86_64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think who have a 20 year old cpu probably does not care about jit too much. However, this was a feature removal, so this could be an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, apparently Debian bookworm's i386 arch requires an i686 CPU, so in theory the PCRE2 packages on that system are meant to work on CPUs without SSE2.
However Debian Trixie bumped that to require a Pentium4, so unconditional use of SSSE2 is allowed. Indeed, it turns out that LLVM no longer supports targeting CPUs older than Pentium4.
Since PCRE2 10.47 isn't going to be packaged for systems older than Trixie, I would be happy to use SSE2 unconditionally.
But it looks easy to add the runtime check when the regex is compiled, so I'm happy either way.
I can't test on such an old CPU anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since PCRE2 10.47 isn't going to be packaged for systems older than Trixie,
FWIW NetBSD/i386 only requires a 486, and 10.47 will be also hopefully packaged and released there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, great. Runtime check on i386 is good then.
|
(Note I read "build fixes" first, and could not find the build fixes :) ) |
|
Agree the PR is badly named, but it is because both commits are otherwise unrelated, and couldn't come with a more clear name for them together; in hindsight; I probably should had make them 2 different PRs. |
| # 50 lines of this file. Please update that if the variables above are moved. | ||
|
|
||
| AC_PREREQ([2.60]) | ||
| AC_PREREQ([2.62]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could bump this to 2.71. I have never tested on anything remotely as old as 2.62. The Autoconf version doesn't affect what OS versions we support, since it's my job to run the Autoconf and check in the results for any release.
To put it another way: it's safer to have a higher version specified, since we won't be testing on anything older
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a job in CI that bootstraps with those older versions for that test is doable, just didn't want to complicate this change with it at this time.
agree, long term we should increase that minimum to something more modern, like 2.69, which is still widely available as it would be needed if the generated/autotools files need patching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake 3.15 is our minimum, released in 2019.
We don't need to support Autoconf versions older than that, surely - it's just about forcing us as contributors/maintainers to update our tools, rather than requiring clients to use newer tools.
Do you really see a need to support anything older than 2.71? Is 2.69 a random version, or does it have particular significance (it seems to be highest version shipped in Ubuntu 20.04, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 2.69 a random version, or does it have particular significance (it seems to be highest version shipped in Ubuntu 20.04, for example).
It is also the most popular version in repology other than the "latest" (2.72) or "legacy"(2.13), and therefore what you would expect in "enterprise" linux like RHEL / Amazon Linux / AlmaLinux or AIX / Solaris (OpenCSW)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, RHEL 8.6 is still within its 10 year extended support, and features Autoconf 2.69.
|
I've noticed that a couple of the architectures (Aarch64 and S390X) seem to have functions with "_sse2_" in their names. A copy-and-paste that forgot to rename? Maybe they should all be just named "_simd_" instead of giving the functions arch-specific names. I also am curious why one of the SIMD functions is blocked on _WIN64? |
|
Yes, probably copy-paste.
Windows has less scratch simd registers than Linux. Using a saved register requires to know quite early that it will be used, and save it. Deciding whether we need it is not trivial. So far nobody complained that JIT is slower on Windows than on Linux, so probably this is not important to anybody. |
Interesting, cool knowledge. Fine by me. |
Make sure we document the correct autoconf version needed for bootstrapping and enable x86 SIMD optimization in all CPUs that support SSE2.
Bootstrapping tested in OpenBSD 7.7 with automake 1.10.