On Sat, 7 Nov 2026 08:12:07 GMT, Shawn Emery <[email protected]> wrote:
>> Mohamed Issa has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge tag 'jdk-28+23' into user/missa-prime/crypto
>>
>> Added tag jdk-28+23 for changeset 2e25332f
>> - Correct SHA512 multi-block intrinsic reference to SHA1 intrinsic source
>> tests.
>> - Remove unnecessary buffers passed into keccak methods in invocation tests.
>> - Pass caught exceptions into constructors of newly thrown exceptions.
>> - Change test flags in some source files to match style of others.
>> - Fix trailing whitespace in InvokeSanityTestBase.java source file.
>> - Modify SHA tests exercise keccak methods.
>
> test/hotspot/jtreg/compiler/intrinsics/sha/TestInvoke.java line 31:
>
>> 29: * @run main/othervm/timeout=600 -Xbatch -XX:-TieredCompilation
>> 30: * --add-opens java.base/sun.security.provider=ALL-UNNAMED
>> 31: * -XX:-UseSHA3Intrinsics -Dmethod=doubleKeccak
>
> The JVM will fail with the `-XX:-UseSHA3Intrinsics `option unless you also
> specify `-XX:+UnlockDiagnosticVMOptions`.
This should be fixed now.
> test/hotspot/jtreg/compiler/intrinsics/sha/TestInvoke.java line 35:
>
>> 33: * @run main/othervm/timeout=600 -Xbatch -XX:-TieredCompilation
>> 34: * --add-opens java.base/sun.security.provider=ALL-UNNAMED
>> 35: * -XX:-UseSHA3Intrinsics -Dmethod=quadKeccak
>
> The JVM will fail with the `-XX:-UseSHA3Intrinsics `option unless you also
> specify `-XX:+UnlockDiagnosticVMOptions`.
This should be fixed now.
> test/hotspot/jtreg/compiler/intrinsics/sha/TestInvoke.java line 51:
>
>> 49: String klass = "sun.security.provider.SHA3Parallel";
>> 50: String method = System.getProperty("method",
>> DOUBLE_KECCAK_METHOD_ID);
>> 51: int iters = (args.length > 1 ? Integer.valueOf(args[0]) : 50000);
>
> Iteration argument would not be read here if it was the only argument.
Thanks for spotting this. I opted to match what Tobias did in
`TestDigest.java`, so the argument is no longer required.
> test/hotspot/jtreg/compiler/testlibrary/sha/predicate/IntrinsicPredicates.java
> line 175:
>
>> 173: public static BooleanSupplier isSHA3QuadKeccakIntrinsicAvailable() {
>> 174: return new AndPredicate(IntrinsicPredicates.COMPILABLE_BY_C2,
>> 175:
>> IntrinsicPredicates.isIntrinsicAvailable("sun.security.provider.SHA3Parallel",
>> "quadKeccak"));
>
> The positive test will fail on AArch64 and non-AVX512 systems as there are no
> stubs generated for quad-Keccak on these systems. Unforetunatly the
> appropriate solution should be implemented in
> `C2Compiler::is_intrinsic_supported`, where the method is checked to see if
> it has been defined and therefore moved out of the unconditional grouping,
> e.g.:
>
>> case vmIntrinsics::_quad_keccak:
>> if (StubRoutines::quad_keccak() == nullptr) return false;
>> break;
This is included now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33709#discussion_r4519520088
PR Review Comment: https://git.openjdk.org/jdk/pull/33709#discussion_r4519522121
PR Review Comment: https://git.openjdk.org/jdk/pull/33709#discussion_r4519545254
PR Review Comment: https://git.openjdk.org/jdk/pull/33709#discussion_r4519547227
