Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v20]

2024-05-24 Thread Scott Gibbons 
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]

2024-05-24 Thread Sandhya Viswanathan 
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]

2024-05-21 Thread Scott Gibbons 
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]

2024-05-21 Thread Scott Gibbons 
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]

2024-05-21 Thread Sandhya Viswanathan 
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]

2024-05-17 Thread Scott Gibbons 
> 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