Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v41]
On Sat, 25 May 2024 06:33:51 GMT, Alan Bateman wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix test; review comments
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 47:
>
>> 45: public class IndexOf {
>> 46:
>> 47: static Random generator = new Random();
>
> @RogerRiggs Would you have cycles to look at Scott's changes to this test? I
> suspect it will need to be re-structured, re-formatted, and commented to get
> into maintainable shape.
I am going to revert my changes to this file as the test
`jdk/java/lang/String/IndexOf.java` covers the code better.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617560447
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v41]
On Sat, 25 May 2024 00:15:03 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix test; review comments > > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 316: > >> 314: // Set up jump tables. Used when needle size <= NUMBER_OF_CASES >> 315: setup_jump_tables(ae, L_returnRBP, L_checkRangeAndReturn, >> L_bigCaseFixupAndReturn, >> 316: _jump_table, _jump_table, _masm); > > We could directly use L_returnError here instead of L_returnRBP. OK > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 476: > >> 474: // Used to check and return value in rbp - usually error >> 475: __ bind(L_returnRBP); >> 476: __ movq(rax, rbp); > > This seems spurious as rax is being overwritten at line 489. Did you intend > to return -1? Removed all references to L_returnRBP. Replaced with L_returnError. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1816: > >> 1814: byte_compare_helper(i + 1, L_loopTop, L_fixup, needle, >> needle_val, hs_ptr, eq_mask, set_bit, >> 1815: rTmp4, ae, _masm); >> 1816: } > > L_checkRange on NoMatch could be L_error instead. Fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614900796 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614903860 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614901577
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v41]
On Fri, 24 May 2024 23:15:26 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 test; review comments
test/jdk/java/lang/StringBuffer/IndexOf.java line 47:
> 45: public class IndexOf {
> 46:
> 47: static Random generator = new Random();
@RogerRiggs Would you have cycles to look at Scott's changes to this test? I
suspect it will need to be re-structured, re-formatted, and commented to get
into maintainable shape.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614383260
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v41]
On Fri, 24 May 2024 23:15:26 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 test; review comments src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 316: > 314: // Set up jump tables. Used when needle size <= NUMBER_OF_CASES > 315: setup_jump_tables(ae, L_returnRBP, L_checkRangeAndReturn, > L_bigCaseFixupAndReturn, > 316: _jump_table, _jump_table, _masm); We could directly use L_returnError here instead of L_returnRBP. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 476: > 474: // Used to check and return value in rbp - usually error > 475: __ bind(L_returnRBP); > 476: __ movq(rax, rbp); This seems spurious as rax is being overwritten at line 489. Did you intend to return -1? src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1816: > 1814: byte_compare_helper(i + 1, L_loopTop, L_fixup, needle, > needle_val, hs_ptr, eq_mask, set_bit, > 1815: rTmp4, ae, _masm); > 1816: } L_checkRange on NoMatch could be L_error instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614172379 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614172021 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614175081
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v41]
> 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 test; review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/be001e2c..b154faee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=40 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=39-40 Stats: 31 lines in 3 files changed: 4 ins; 13 del; 14 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
