On Fri, 6 Nov 2026 23:42:54 GMT, Sandhya Viswanathan <[email protected]>
wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comments from Sandhya
>
> src/hotspot/cpu/x86/assembler_x86.cpp line 1578:
>
>> 1576:
>> 1577: void Assembler::vaesenc(XMMRegister dst, XMMRegister nds, XMMRegister
>> src, int vector_len) {
>> 1578: assert(vector_len == AVX_128bit ? (VM_Version::supports_aes() &&
>> VM_Version::supports_avx()) || // VEX
>
> Wondering if we should restrict this and other vaes instructions to only
> supports_vaes().
> i.e. remove VM_Version::supports_aes() && VM_Version::supports_avx().
> It is bit conservative but meets our usage.
You are right, missed that; we now have AESAssembler, aes/vaes routing happens
there already.
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 161:
>
>> 159: 0x0000000000000000ULL, 0x0000000000000000ULL,
>> 160:
>> 161: // [0:15] Sequence 0-8
>
> Should this be Sequence 0-3
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 200:
>
>> 198: // 1 1 0 | 0 1 (impossible; i=1)
>> 199: // 1 1 1 | 1 1 (impossible; i=1)
>> 200: //
>
> Unclear what is ctr_i, n, i here.
Redid the comment, hopefully clearer now
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 345:
>
>> 343: }
>> 344:
>> 345: void vpxor(XMMRegister dst, XMMRegister nds, Address src, int
>> vector_len, XMMRegister scratch = xmm1) {
>
> Wonder if it would be good to pass scratch always. When not needed xnoreg
> could be passed.
Yeah, in hindsight this is very dangerous. Made it explicit where it would be
needed (dst == nds)
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 352:
>
>> 350: if (dst != nds) {
>> 351: movdqu(dst, src);
>> 352: pxor(dst, nds);
>
> Assert here:
> assert(scratch == noreg);
shouldnt care if scratch was passed in and not used, no?
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 356:
>
>> 354: movdqu(scratch, src);
>> 355: pxor(dst, scratch);
>> 356: }
>
> Assert here:
> assert(scratch != noreg);
> assert_different_registers(dst, scratch);
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 425:
>
>> 423: // incr rest of counters
>> 424: // LOOP:
>> 425: // full aesrounds(4 per key)
>
> There is an assumption here that the maxParallel is 4.
changed comments
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 509:
>
>> 507: const Register len = rsi; // c_rarg4
>> 508: const Register savedCtr = rdi;// c_rarg5
>> 509: const Register usedAddr = r10;// c_rarg6
>
> There is no c_rarg6 so comment needs update.
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 519:
>
>> 517: const Register len = c_rarg4;
>> 518: const Register savedCtr = c_rarg5;
>> 519: const Register usedAddr = r10; // c_rarg6
>
> There is no c_rarg6 so comment needs update.
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 541:
>
>> 539:
>> 540: // Constants/parameters
>> 541: int totalRegs = vector_len == Assembler::AVX_512bit ||
>> VM_Version::supports_avx512vl() ? 32 : 16;
>
> It might be easier to say supports_evex() instead of (Assembler::AVX_512bit
> || VM_Version::supports_avx512vl()).
hmm.. this also covers vector_len==128, I think I do need
VM_Version::supports_avx512vl(). But Assembler::AVX_512bit can go.
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 565:
>
>> 563: Ctr[i] = _next;
>> 564: }
>> 565: for (int i = 0; i<15 && _next->is_valid(); i++, _next =
>> _next->successor()) {
>
> Should this be i < keyRegs?
Oh.. that's much better. 15 here was for the length of the keys array. Though
it would result with unusable registers on avx2, which was confusing. This is
cleaner.
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 577:
>
>> 575: }
>> 576:
>> 577: // if (len == 0) return len
>
> // if (len <=0) return len
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 581:
>
>> 579: __ cmpl(len, 0);
>> 580: __ jcc(Assembler::belowEqual, ExitLabel);
>> 581:
>
> We could move this earlier, say at line 568.
had to rearange a few things to save some stack operations. I could save the
Windows pushes too, but would not look as clean
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 664:
>
>> 662: }
>> 663: };
>> 664: keysAndRounds(true, 0, 10, -1, 2);
>
> May be add a comment here to say why -1?
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 693:
>
>> 691:
>> 692: // Increment is now a constant step, 2 Ctrs at a time
>> 693: __ vpbroadcastq(Increment, 2*blocksPerReg, vector_len, tmp);
>
> May be better to say parallel*blocksPerReg?
This one is actually a 2 because we have 'two registers at a time' (independent
of parallel: CtrLow, CtrHigh need to be 'stepped over')
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 926:
>
>> 924:
>> 925: // record the stub entry and end
>> 926: stubgen->store_archive_data(stub_id, start, __ pc());
>
> store_archive_data should be called after __ ret(0).
done
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 954:
>
>> 952: if (UseAESCTRIntrinsics) {
>> 953: int vector_len = Assembler::AVX_128bit;
>> 954: if (VM_Version::supports_vaes() && VM_Version::supports_avx512bw()
>> && VM_Version::supports_avx512vl()) {
>
> We also need avx512dq() for evbroadcasti64x2.
missed that. But switched to evbroadcasti32x4 so I dont have to increase
dependency.
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 959:
>
>> 957: vector_len = Assembler::AVX_256bit;
>> 958: } else {
>> 959: assert(VM_Version::supports_aes() &&
>> VM_Version::supports_sse4_1(), "AESNI intrinsic unsupported");
>
> This should be VM_Version::supports_sse4_2() (pcmpgtq needs sse4_2).
Changed. As far as I can tell, no coverage is lost, machines with aesni had
sse4.2 at least.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4540484123
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4543022043
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4486093326
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4517106376
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4517281115
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4517115173
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4484141781
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4515863353
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4515864031
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4484101157
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4515937756
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4486568913
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4486577815
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4517534084
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4486616629
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4515803439
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4540503665
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4540541895
