On Mon, 17 Mar 2025 19:22:41 GMT, Volodymyr Paprotski <vpaprot...@openjdk.org>
wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Made the intrinsics test separate from the pure java test.
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 45:
>
>> 43: // Constants
>> 44: //
>> 45: ATTRIBUTE_ALIGNED(64) static const uint32_t dilithiumAvx512Consts[] = {
>
> This is really nitpicking.. but could had loaded constants inline with `movl`
> without requiring an ExternalAddress()?
>
> Nice to have constants together, only complaint is we have 'magic offsets' in
> ASM to reach in for particular one..
>
> This one isnt too bad, offset of 32bits is easy to inspect visually
> (`dilithiumAvx512ConstsAddr()` could take a parameter perhaps)
I added symbolic names for the indexes.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23860#discussion_r2021149647
