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

[David Holmes]

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.

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

---

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.

---

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.

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:

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;

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

4352       = (target & 0xffffffffUL) | ((uint64_t)pc() & 0xffff00000000UL);

Should those constants be ULL? (they seem oddly expressed for 64-bit 
constants)

---

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.

---

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".

---

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

montgomery_multiply: not at all clear why these should be julong

---

Thanks,
David
-----