Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
On Thu, 30 May 2024 16:03:29 GMT, Emanuel Peter wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix bug number in tests
>
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.
>
> Is the 2023 year intentional? I don't know your policy, so you can just
> ignore this ;)
I started this in November :-)
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 334:
>
>> 332: // NUMBER_OF_CASES (currently 10) needle sizes for both big and
>> small. There are special
>> 333: // routines for handling needle sizes > NUMBER_OF_CASES
>> (L_{big,small}CaseDefault). These
>> 334: // cases use C@'s arrays_equals() to compare the needle to the
>> haystack. The small cases
>
> Suggestion:
>
> // cases use C2's arrays_equals() to compare the needle to the haystack.
> The small cases
>
> Randomly spotted this.
Fixed.
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 773:
>
>> 771: // jae done
>> 772: //
>> 773: // Final index of start of needle @((16 - (ndlLen %16)) & 0xf) << 1
>
> What is the meaning of the `@`? Maybe `at`. I'd use the same consistently
Changed to "at".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034441
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034583
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034821
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
On Thu, 30 May 2024 15:48:50 GMT, Scott Gibbons wrote:
>> Re-write the IndexOf code without the use of the pcmpestri instruction, only
>> using AVX2 instructions. This change accelerates String.IndexOf on average
>> 1.3x for AVX2. The benchmark numbers:
>>
>>
>> Benchmark Score
>> Latest
>> StringIndexOf.advancedWithMediumSub 343.573317.934
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081 1053.96
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541
>> 1.980027943x
>> StringIndexOf.constantPattern9.361 11.906
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess 4.216 4.218
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133 3.216
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761
>> 1.000265957x
>> StringIndexOf.success9.186
>> 9.713 1.057369911x
>> StringIndexOf.successBig 14.34146.343
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String 7132.541
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char 16013.389 16162.437
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String 7386.12314771.622
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671 9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix bug number in tests
Ok, now it is good for me. But I would definately wait with integration for
after the fork next week.
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.
Is the 2023 year intentional? I don't know your policy, so you can just ignore
this ;)
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 334:
> 332: // NUMBER_OF_CASES (currently 10) needle sizes for both big and small.
> There are special
> 333: // routines for handling needle sizes > NUMBER_OF_CASES
> (L_{big,small}CaseDefault). These
> 334: // cases use C@'s arrays_equals() to compare the needle to the
> haystack. The small cases
Suggestion:
// cases use C2's arrays_equals() to compare the needle to the haystack. The
small cases
Randomly spotted this.
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 773:
> 771: // jae done
> 772: //
> 773: // Final index of start of needle @((16 - (ndlLen %16)) & 0xf) << 1
What is the meaning of the `@`? Maybe `at`. I'd use the same consistently
-
Marked as reviewed by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16753#pullrequestreview-2088739965
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621015782
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621017548
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621019611
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
> Re-write the IndexOf code without the use of the pcmpestri instruction, only > using AVX2 instructions. This change accelerates String.IndexOf on average > 1.3x for AVX2. The benchmark numbers: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Fix bug number in tests - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/57e115d7..6eae46e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=50 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=49-50 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
