On Thu, 5 Oct 2023 18:46:46 GMT, Vladimir Kozlov <k...@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
