On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons <sgibb...@openjdk.org> 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.573 317.934
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub1 1039.081 1053.96
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub2 55.828 110.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.76 3.761
>> 1.000265957x
>> StringIndexOf.success 9.186
>> 9.713 1.057369911x
>> StringIndexOf.successBig 14.341 46.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.359 0.962260014x
>> StringIndexOfChar.latin1_Short_char 16013.389 16162.437
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String 7386.123 14771.622
>> 1.999915517x
>> 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:
>
> Rearrange; add lambdas for clarity
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 1174:
> 1172: // Alignment specifying the maximum number of allowed bytes to pad.
> 1173: // If padding > max, no padding is inserted.
> 1174: void MacroAssembler::p2align(int modulus, int maxbytes) {
We could pass offset() as an argument to p2align. Basically have three
arguments to p2align(modulus, target, maxbytes). Also maybe rename p2align as
align then?
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 208:
> 206:
> ////////////////////////////////////////////////////////////////////////////////////////
> 207:
> ////////////////////////////////////////////////////////////////////////////////////////
> 208: if (VM_Version::supports_avx2()) { // AVX2 version
Instead of the if check here, it would be better to do an assert here:
assert (VM_Version::supports_avx2(), "Needs AVX2 support");
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 233:
> 231:
> ////////////////////////////////////////////////////////////////////////////////////////
> 232:
> ////////////////////////////////////////////////////////////////////////////////////////
> 233:
This comment can go right before the method start. Also good to add in the
comment the native function parameters.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 238:
> 236: const Register needle = rdx;
> 237: const Register needle_len = rcx;
> 238:
This is the calling convention on Linux. How is windows platform handled?
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 260:
> 258: // const XMMRegister save_rcx = xmm11;
> 259: // const XMMRegister save_r8 = xmm12;
> 260:
This could be removed?
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 279:
> 277: fnptrs[isLL ? StrIntrinsicNode::LL
> 278: : isUU ? StrIntrinsicNode::UU
> 279: : StrIntrinsicNode::UL] = __ pc();
Could this not be simplified as:
fnptrs[ae] = __ pc();
src/hotspot/share/opto/library_call.cpp line 1263:
> 1261: if (result != nullptr) {
> 1262: // The result is index relative to from_index if substring was
> found, -1 otherwise.
> 1263: // Generate code which will fold into cmove.
Any reason to remove this comment?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591547667
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591612417
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591613215
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591617528
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591607921
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591618222
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1591554296
