RFR: 8248414: AArch64: Remove uses of long and unsigned long ints

[Andrew Haley]

On 08/07/2020 08:29, David Holmes wrote:
> Hi Andrew,
>
> On 27/06/2020 1:43 am, Andrew Haley wrote:
>> All of the uses of long and unsigned long int should be replaced with
>> more appropriate types. Where signed and unsigned 64-bit integers are
>> required, use int64_t. Where pointers are required, use
>> intptr_t. Where Java longs are required, use jlong.
>>
>> http://cr.openjdk.java.net/~aph/8248414/
>
> Generally okay but a few comments/queries below.
>
> The change to add all the:
>
> + // This pattern is automatically generated from aarch64_ad.m4
> + // DO NOT EDIT ANYTHING IN THIS SECTION OF THE FILE
>
> headers in the ad file made it much harder to spot the actual type changes.

My apologies, but we've had several instances of people editing
auto-generated patterns. I resolved to make the DO NOT EDIT changes
next time anything in that part of the file changed.

> src/hotspot/cpu/aarch64/assembler_aarch64.cpp
>
> 1704 bool Assembler::operand_valid_for_add_sub_immediate(*int64_t* imm) {
> 1705   bool shift = false;
> 1706   *julong* uimm = uabs(imm);
>
> That seems a strange mix of C types and Java types

Ummm, true. I did it because uabs() is defined to take a jlong and
return a julong. I will cast the returned value to uint64_t.

> src/hotspot/cpu/aarch64/assembler_aarch64.hpp
>
>     int64_t offset = (dest - pc()) >> 2;
>
> It isn't immediately obvious to me why the offset should be signed in
> these cases whereas in other parts of the code offsets are unsigned.
> Just an observation though as you are not changing the signedness.

That's how the processor works: some offsets are signed, some are
unsigned. I've tried to make it so that the signedness of the offset
is always the signedness of the instruction. I may have made some
mistakes in this area.

> ---
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
>
>   136         Instruction_aarch64::patch(branch + 4, 20, 5,
> (uint64_t)target >> 32);
>   137          intptr_t dest = ((int64_t)target & 0xffffffffL) |
> ((int64_t)branch & 0xffff00000000L);
>   138         intptr_t pc_page = (int64_t)branch >> 12;
>   139         intptr_t adr_page = (int64_t)dest >> 12;
>
> There's an extra leading space on line 137.

OK.

> It's not obvious to me why the casts are to int64_t and the variable
> assigned to is intptr_t, especially when just up from this we have:

It's not obvious to me either! I'll try to be more consistent.

> 96       uint64_t dest = (uint64_t)target;
> 97       uint64_t pc_page = (uint64_t)branch >> 12;
> 98       uint64_t adr_page = (uint64_t)target >> 12;
>
> Also should those constants be LL for good measure?
>
> 1720   uint64_t mask = (1ul << shift) - 1;

Yes! Well spotted.

> Making the mask 64-bit instead of 32-bit seems like a functional change.
> Also the 1ul should be 1ull shouldn't it?

I'll take another look. I don't wany to make any functional changes in
this patch.

> 4352       = (target & 0xffffffffUL) | ((uint64_t)pc() & 0xffff00000000UL);
>
> Should those constants be ULL? (they seem oddly expressed for 64-bit
> constants)

OK.

> ---
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64_log.cpp
>
> Not obvious why the constants should be Java types.
>
> ---
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64_trig.cpp
>
> Not obvious why the constants should be Java types.
>
> ! // *              jlong double t,w,r_head, r_tail;
>
> The "long" is part of "long double" and shouldn't have been changed to
> jlong.

OK.

> ---
>
> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
>
> !     // However to make thing extra confusing. Because we can fit a
> jlong/double in
> !           // jlong/double in gpr
>
> I think it was clear from context that "long" was referring to the Java
> type. If you wanted to be sure I'd suggest writing "Java long/double".

OK.

Thank you for such a careful and heplful review.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671