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

[Steve Dohrmann]

On Thu, 25 Apr 2024 18:21:28 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> Steve Dohrmann has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   bug fix in other ::prefix_rex2
>
> src/hotspot/cpu/x86/assembler_x86.cpp line 648:
> 
>> 646:       }
>> 647:     } else if ((base_enc & 0x7) == 4) {
>> 648:       // rbp | r12 | r20 | r28
> 
> Comment should be:
> // rsp | r12 | r20 | r28

Thanks, done.

> src/hotspot/cpu/x86/assembler_x86.cpp line 670:
> 
>> 668:     } else {
>> 669:       // [base + disp]
>> 670:       // !(rbp | r12 | r20 | r28) were handled above
> 
> comment should be:
> // (rsp | r12 | r20 | r28) were handled above

Thanks, done.

> src/hotspot/cpu/x86/assembler_x86.cpp line 12897:
> 
>> 12895:   if (adr.index_needs_rex2()) {
>> 12896:     assert(false, "prefix(Register dst, Address adr) does not support handling of an X");
>> 12897:   }
> 
> this could be written as:
> assert(!adr.index_needs_rex2(), "prefix(Register dst, Address adr) does not support handling of an X");

Very nice.  Thank you, done.

> src/hotspot/cpu/x86/assembler_x86.cpp line 13839:
> 
>> 13837: void Assembler::movsbq(Register dst, Address src) {
>> 13838:   InstructionMark im(this);
>> 13839:   int prefix = get_prefixq(src, dst, true /* page1 */);
> 
> We are not consistent in the comment is_map1, M0, page1 all refer to the same thing. Also some places there is no comment that the true is for is_map1.

Thank you.  Naming is now consistent with APX spec naming (is_map1 for the bool argument and M0 for the bit name).  I added inline comment  /* is_map1 */ for calls that pass the boolean argument.

> src/hotspot/cpu/x86/assembler_x86.cpp line 14479:
> 
>> 14477:     _input_size_in_bits = input_size_in_bits;
>> 14478:   }
>> 14479: }
> 
> New line missing at the end of file.

Thanks, done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1581510510
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1581511199
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1581511569
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1581511880
PR Review Comment: https://git.openjdk.org/jdk/pull/18476#discussion_r1581512106