Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
On Tue, 21 May 2024 22:39:42 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressing lots of comments. Interim commit. > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4737: > >> 4735: bind(COMPARE_BYTE); >> 4736: } else { >> 4737: lea(ary1, Address(ary1, expand_ary2 ? 4 : 2)); > > This change is not required. expand_ary2 code doesn't come here. Right. Fixed. > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1233: > >> 1231: __ andq(eq_mask, rTmp); >> 1232: >> 1233: __ testl(eq_mask, eq_mask); > > Mismatch of operation size q vs l: andq and testl. Fixed. > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1623: > >> 1621: >> >> 1622: // >> 1623: // Small haystack (<32 bytes) switch > > This should be <= 32 bytes. Fixed. > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1709: > >> 1707: // XMM_BYTE_K - last element of needle, broadcast >> 1708: // >> 1709: // The haystack is >= 32 bytes > > Should this be > 32 bytes? Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614114763 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127986 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127889 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127781
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
On Fri, 17 May 2024 23:47:45 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: > > Addressing lots of comments. Interim commit. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4737: > 4735: bind(COMPARE_BYTE); > 4736: } else { > 4737: lea(ary1, Address(ary1, expand_ary2 ? 4 : 2)); This change is not required. expand_ary2 code doesn't come here. src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1233: > 1231: __ andq(eq_mask, rTmp); > 1232: > 1233: __ testl(eq_mask, eq_mask); Mismatch of operation size q vs l: andq and testl. src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1623: > 1621: > > 1622: // > 1623: // Small haystack (<32 bytes) switch This should be <= 32 bytes. src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1709: > 1707: // XMM_BYTE_K - last element of needle, broadcast > 1708: // > 1709: // The haystack is >= 32 bytes Should this be > 32 bytes? - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609023624 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609043720 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609160143 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609163535
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
On Fri, 17 May 2024 23:47:45 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: > > Addressing lots of comments. Interim commit. Comment on behalf of @sviswa7 : Unclear whether `size` in `byte_compare_helper` is intended to be in bytes or in elements. Please check its consistency. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2123736900
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
On Tue, 21 May 2024 18:03:41 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressing lots of comments. Interim commit. > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4648: > >> 4646: vpxor(vec1, vec2); >> 4647: >> 4648: vptest(vec1, vec1); > > These should be only 128 bit: >pxor(vec1, vec2); >ptest(vec1, vec1); Fixed > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1351: > >> 1349: assert_different_registers(needle, needleVal); >> 1350: >> 1351: bool isLL = (ae == StrIntrinsicNode::LL); > > isLL not used in this function. Fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609164643 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609164578
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
On Fri, 17 May 2024 23:47:45 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: > > Addressing lots of comments. Interim commit. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4648: > 4646: vpxor(vec1, vec2); > 4647: > 4648: vptest(vec1, vec1); These should be only 128 bit: pxor(vec1, vec2); ptest(vec1, vec1); src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1351: > 1349: assert_different_registers(needle, needleVal); > 1350: > 1351: bool isLL = (ae == StrIntrinsicNode::LL); isLL not used in this function. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1608732591 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1605624430
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]
> 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: Addressing lots of comments. Interim commit. - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/fb4da92a..9a861979 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=19 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=18-19 Stats: 1639 lines in 9 files changed: 429 ins; 683 del; 527 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