RFR: 8328998: Encoding support for Intel APX extended general-purpose registers [v13]

[Steve Dohrmann]

On Fri, 3 May 2024 14:14:28 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Steve Dohrmann 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 20 additional commits since the last revision:
>> 
>>  - update for egpr use: bzhil(R,R,R), btq(R,R), btq(R,imm)
>>  - Merge branch 'master' into apx-encoding-pr
>>  - Update full name
>>  - simplification and fix asserts in ldmxcsr, stmxcsr, and emit_prefix_and_int8
>>  - remove is_map1 comment for addb, andb, movb, orb, testb, xchgb, xorb
>>  - fix stmxcrs REX2 branch, add asserts to SHA instructions
>>  - fixes: pp bits in crc32, REX2 branch in ldmxcsr
>>  - add egpr support for popcntq(R,A), cvttsd2siq(R,A), popq(R)
>>  - fix 4 more src_is_gpr = true cases, add asserts to check for UseAPX
>>  - fix is_gpr arg on two functions with reversed src / dst operands
>>  - ... and 10 more: https://git.openjdk.org/jdk/compare/dc7f6595...7b3e8ec7
>
> src/hotspot/cpu/x86/assembler_x86.cpp line 1726:
> 
>> 1724: 
>> 1725: void Assembler::blsrl(Register dst, Register src) {
>> 1726:   assert(VM_Version::supports_bmi1(), "bit manipulation instructions not supported");
> 
> We should extend assertion checks based on register encodings and feature detection upfront using ` VM_Version::supports_apx_f() ` part of [PR#18562](https://github.com/openjdk/jdk/pull/18562)  once it lands OR you can merge that pull request with this patch if you find appropriate.

Agree that asserts should be extended.  Maybe it would be better to do so in a subsequent PR with feature detection in place.

> src/hotspot/cpu/x86/assembler_x86.cpp line 2839:
> 
>> 2837: void Assembler::kmovwl(KRegister dst, KRegister src) {
>> 2838:   assert(VM_Version::supports_evex(), "");
>> 2839:   InstructionAttr attributes(AVX_128bit, /* rex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 
> Suggestion:
> 
>   InstructionAttr attributes(AVX_128bit, /* rex_w */ false, /* legacy_mode */ true, /* no_mask_reg */ true, /* uses_vl */ false);
> 
> 
> No GPR operand here.

Thanks, made the change back.

> src/hotspot/cpu/x86/assembler_x86.cpp line 2860:
> 
>> 2858: void Assembler::kmovql(KRegister dst, KRegister src) {
>> 2859:   assert(VM_Version::supports_avx512bw(), "");
>> 2860:   InstructionAttr attributes(AVX_128bit, /* rex_w */ true, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 
> Suggestion:
> 
>   InstructionAttr attributes(AVX_128bit, /* rex_w */ true, /* legacy_mode */ true, /* no_mask_reg */ true, /* uses_vl */ false);

Thanks, made the change back.

> src/hotspot/cpu/x86/assembler_x86.cpp line 6556:
> 
>> 6554:   assert(VM_Version::supports_bmi1(), "tzcnt instruction not supported");
>> 6555:   emit_int8((unsigned char)0xF3);
>> 6556:   int encode = prefixq_and_encode(dst->encoding(), src->encoding(), true /* is_map1 */);
> 
> FTR, Quoting relevant except from section 3.1.2.1 of APX specification.
> “REX2 must be the last prefix. The byte following it is interpreted as the main opcode byte in the opcode map indicated by M0. The 0x0F escape byte is neither needed nor allowed.”

Thanks, understand.  The prefixq_and_encode function used above does not emit the 0x0F opcode prefix for map1 instructions encoded with the REX2 scheme.

> src/hotspot/cpu/x86/assembler_x86.hpp line 536:
> 
>> 534:     REXBIT_X  = 0x02,
>> 535:     REXBIT_R  = 0x04,
>> 536:     REXBIT_W  = 0x08,
> 
> Suggestion:
> 
>     REX2BIT_B  = 0x01,
>     REX2BIT_X  = 0x02,
>     REX2BIT_R  = 0x04,
>     REX2BIT_W  = 0x08,
> 
> Name change suggestion since these bits are part of REX2 prefix.

It's true that the REXBIT constants are currently only used in REX2 encoding code.  The reason for choosing the REXBIT name for those four values was that they do refer to REX encoding bits and, if bit-wise refactoring of the existing REX encoding code was to be done later, the REXBIT names would make more sense there.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1589636409
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1589635515
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1589635298
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1589635856
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1589635146