RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v41]

[Srinivas Vamsi Parasa]

On Thu, 5 Oct 2023 18:46:46 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

> In general it looks good. I have some code style comments and file name change request. After you fix that I will need to rerun testing for it before approval.

Hello Vladimir (@vnkozlov),

Thank you for the suggestions related to the code style and formatting!
Please see all the suggested changes incorporated into the latest commit that was just pushed.

Thanks,
Vamsi

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4183:
> 
>> 4181: 
>> 4182:   // Load x86_64_sort library on supported hardware to enable avx512 sort and partition intrinsics
>> 4183:     if (UseAVX > 2 && VM_Version::supports_avx512dq()) {
> 
> Indention (spacing) is wrong for lines 4183-4190 after you moved check.

Fixed the indentation. Please see the latest commit that was pushed.

> src/hotspot/share/opto/library_call.cpp line 5372:
> 
>> 5370: bool LibraryCallKit::inline_array_partition() {
>> 5371: 
>> 5372:   address stubAddr = nullptr;
> 
> Move this declaration to first assignment at line 5387.

Fixed the declaration. Please see the latest commit that was pushed.

> src/hotspot/share/opto/library_call.cpp line 5374:
> 
>> 5372:   address stubAddr = nullptr;
>> 5373:   const char *stubName;
>> 5374:   stubName = "array_partition_stub";
> 
> It could one line.

Fixed the assignment. Please see the latest commit that was pushed.

> src/hotspot/share/opto/library_call.cpp line 5400:
> 
>> 5398: 
>> 5399:   // create the pivotIndices array of type int and size = 2
>> 5400:   Node* pivotIndices = nullptr;
> 
> Move to assignment at line 5403.

Moved the assignment. Please see the latest commit that was pushed.

> src/hotspot/share/opto/library_call.cpp line 5414:
> 
>> 5412:   make_runtime_call(RC_LEAF|RC_NO_FP, OptoRuntime::array_partition_Type(),
>> 5413:                     stubAddr, stubName, TypePtr::BOTTOM,
>> 5414:                     obj_adr, elemType, fromIndex, toIndex, pivotIndices_adr, indexPivot1, indexPivot2);
> 
> May be put `indexPivot*` argument on new line for fit screen better.

Fixed the indentation. Please see the latest commit that was pushed.

> src/java.base/linux/native/libsimdsort/avx512-32bit-qsort.hpp line 4:
> 
>> 2:  * Copyright (c) 2021, 2023, Intel Corporation. All rights reserved.
>> 3:  * Copyright (c) 2021 Serge Sans Paille. All rights reserved.
>> 4:  * Intel x86-simd-sort source code.
> 
> I don't think this line here and in other new files will pass our Copyright header checker. Do you need this line here? You do have comment below `This implementation is based on x86-simd-sort`.
> `DO NOT ALTER ..` line should immediately follow `Copyright` lines.

Fixed the copyright headers. Please see the latest commit that was pushed.

> src/java.base/linux/native/libsimdsort/avxsort_linux_x86.cpp line 1:
> 
>> 1: /*
> 
> I think this file name should follow pattern of other files: `avx512-linux-qsort.cpp

Changed the name of the file as suggested. Please see the latest commit that was pushed.

> src/java.base/share/classes/java/util/DualPivotQuicksort.java line 418:
> 
>> 416: 
>> 417:         /*
>> 418:         * The first and the last elements to be sorted are moved
> 
> Misaligned `/*` here and several places later. `*` should be aligned.

Fixed the alignment for `/*` and `*` in various places. Please see the latest commit that was pushed.

> src/java.base/share/classes/java/util/DualPivotQuicksort.java line 1353:
> 
>> 1351:         /*
>> 1352:             * Swap the pivot into its final position.
>> 1353:             */
> 
> Indent

Fixed the indentation. Please see the latest commit that was pushed.

> src/java.base/share/classes/java/util/DualPivotQuicksort.java line 2941:
> 
>> 2939:         /*
>> 2940:             * Swap the pivot into its final position.
>> 2941:             */
> 
> indent

Fixed the indentation. Please see the latest commit that was pushed.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1749797401
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348105761
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106048
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348105919
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106228
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106326
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348107205
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106960
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106765
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106515
PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1348106465