Give the Alpha and x86 start-bits scans one interface - #983
Merged
NWilson merged 1 commit intoSep 12, 2026
Merged
Conversation
The Alpha CMPBGE scan (PCRE2Project#921) and the x86-64 vector scan (PCRE2Project#941) each added a fast_forward_start_bits_simd() behind JIT_HAS_FAST_FORWARD_START_BITS_SIMD, with a signature and a call site of their own. The two never conflicted in git, so once both had landed the Alpha call site was passing two arguments to the function x86-64 declares with four, and the build failed everywhere. Keep the x86-64 interface, the more capable of the two: the Alpha scan takes the offset of the bitmap within the match and the flag for whether bit 255 stands for the code units above it, clamps STR_END against match_end_ptr itself instead of leaving that to the caller, and restarts on a candidate that is not a UTF character boundary. Its second call site then has nothing left to do. pcre2_jit_test and RunTest pass at all three code unit widths on x86-64 and under qemu-alpha.
Member
|
I understand. There are two implementations of fast_forward_start_bits_simd: the x86 one, and the Alpha one. You are keeping both of them, but making their signatures match. The callsite for the two implementations was added in nearly the same place, but we were unlucky that it didn't cause a conflict. Thank you very much @mattst88! |
Member
|
The change very much looks good to me, after reading and thinking a bit. It's also super low-risk, because it only removes the block of code added for the sake of Alpha, and changes the Alpha code. So this cannot break anything on x86/amd64. (Anything broken on those arches would have been already broken, but Zoltan and Carlo reviewed that.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the x86 build failure reported in #941 (comment).
#921 and #941 each added a
fast_forward_start_bits_simd()behindJIT_HAS_FAST_FORWARD_START_BITS_SIMD, with a signature and a call site of its own: two arguments for the Alpha CMPBGE scan, four for the x86-64 vector scan. The two never touched the same lines, so git merged both cleanly and CI did not re-run either once the other had landed. With both in,fast_forward_start_bits()holds two call sites and the Alpha one passes two arguments to the function x86-64 declares with four:This keeps the x86-64 interface, the more capable of the two. The Alpha scan takes the offset of the bitmap within the match and the flag for whether bit 255 stands for the code units above it, clamps STR_END against
match_end_ptritself instead of leaving that to the caller, and restarts on a candidate that is not a UTF character boundary. Its second call site then has nothing left to do, and the borrow of R5 to preserve RETURN_ADDR acrossemit_alpha_ctz8()goes away with it.Testing:
pcre2_jit_testandRunTestpass at all three code unit widths.firstlineanduse_offset_limit(the two options that setmatch_end_ptr): identical output on both architectures.-Wall -Wextrafor both architectures, with JIT on and off.One thing I left alone, since it wants a measurement rather than a guess: the x86-64 scan declines a class covering more than
X86_START_BITS_MAX_COVEREDcode units, on the grounds that a dense class usually stops the loop at once while the scan has already tested a whole block. The Alpha scan has no equivalent cap, only its three constant budget, so now that it reaches the offset call site it vectorizes classes x86-64 turns down,.{3}[a-zA-Z]among them. The argument for a cap is weaker at eight bytes than at sixteen, but it is not absent. I would rather addALPHA_START_BITS_MAX_COVEREDonce I have numbers from real hardware than pick a threshold here.