On Tue, 25 Nov 2025 02:50:41 GMT, Jatin Bhateja <[email protected]> wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> spelling
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 365:
>
>> 363:
>> 364: static void loadXmms(const XMMRegister destinationRegs[], Register
>> source, int offset,
>> 365: int vector_len, MacroAssembler *_masm, int
>> regCnt = -1, int memStep = -1) {
>
> Suggestion:
>
> int vector_len, MacroAssembler *_masm, int regCnt = -1,
> int memStep = -1) {
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 381:
>
>> 379:
>> 380: static void storeXmms(Register destination, int offset, const
>> XMMRegister xmmRegs[],
>> 381: int vector_len, MacroAssembler *_masm, int
>> regCnt = -1, int memStep = -1) {
>
> Suggestion:
>
> int vector_len, MacroAssembler *_masm, int regCnt = -1,
> int memStep = -1) {
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 659:
>
>> 657: // zetas (int[128*8]) = c_rarg1
>> 658: static address generate_dilithiumAlmostInverseNtt_avx(StubGenerator
>> *stubgen,
>> 659: int vector_len,MacroAssembler
>> *_masm) {
>
> Fix indentation
I dont think this is any better:
static address generate_dilithiumAlmostInverseNtt_avx(StubGenerator *stubgen,
int vector_len,
MacroAssembler *_masm) {
I prefer more lines on the screen instead. I also didn't see anything in
hotspot-style.md specifically on function declaration style so figure it is up
to me. Did add a space after the coma.
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 718:
>
>> 716:
>> 717: // Constants for shuffle and montMul64
>> 718: __ mov64(scratch, 0b1010101010101010);
>
> 64 bit constant suffix
Note the `0b` prefix.
`0b0000000000000000000000000000000000000000000000000101010101010101UL` is
worse. And the very next line is using the constant as a 16bit value
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 901:
>
>> 899: // poly2 (int[256]) = c_rarg2
>> 900: static address generate_dilithiumNttMult_avx(StubGenerator *stubgen,
>> 901: int vector_len, MacroAssembler
>> *_masm) {
>
> Fix indentation
as above
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 939:
>
>> 937: vector_len, scratch); // 2^64 mod q
>> 938: if (vector_len == Assembler::AVX_512bit) {
>> 939: __ mov64(scratch, 0b0101010101010101);
>
> Add long constant suffix
as above
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 985:
>
>> 983: // constant (int) = c_rarg1
>> 984: static address generate_dilithiumMontMulByConstant_avx(StubGenerator
>> *stubgen,
>> 985: int vector_len, MacroAssembler
>> *_masm) {
>
> Fix indentation
as above
> src/hotspot/cpu/x86/stubGenerator_x86_64_dilithium.cpp line 1026:
>
>> 1024: __ evpbroadcastd(constant, rConstant, Assembler::AVX_512bit); //
>> constant multiplier
>> 1025:
>> 1026: __ mov64(scratch, 0b0101010101010101); //dw-mask
>
> Constant suffix
as above
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2561127351
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2561128486
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2560573897
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2560581463
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2560583718
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2560585705
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2560635291
PR Review Comment: https://git.openjdk.org/jdk/pull/28136#discussion_r2561171198
