> On Jun 12, 2019, at 5:35 AM, Nick Gasson <nick.gas...@arm.com> wrote:
>
> Hi Andrew,
>
> Sorry for the delay, I've put an updated webrev here:
>
> http://cr.openjdk.java.net/~ngasson/8224851/webrev.1/
Since I've been commenting on some parts, I decided I might as well
look at all of it. So here's a review. I don't need to re-review any
of the suggested changes.
------------------------------------------------------------------------------
src/hotspot/cpu/aarch64/frame_aarch64.cpp
772 reg_map = (RegisterMap*)os::malloc(sizeof(RegisterMap), mtNone);
Use NEW_C_HEAP_OBJ(RegisterMap, mtNone)
------------------------------------------------------------------------------
src/hotspot/cpu/aarch64/macroAssembler_aarch64_trig.cpp
384 fcmpd(v26, 0.0); // if NE then jx == 2.
else it's 1 or 0
and
388 fcmpd(v7, 0.0); // v7 == 0 => jx = 0.
Else jx = 1
EOL comments no longer aligned with others nearby.
------------------------------------------------------------------------------
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp
180 if ((p = strchr(buf, ':'))) {
Hotspot Coding Style says not to use implicit bool, instead using
explicit comparisons, e.g. this should be
180 if ((p = strchr(buf, ':')) != NULL) {
That would have avoided the warning in the first place.
And yes, I know there are lots of violations of that guideline.
------------------------------------------------------------------------------
src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp
43 #ifdef __clang__
44 D res = __atomic_add_fetch(dest, add_value, __ATOMIC_RELEASE);
45 FULL_MEM_BARRIER;
46 return res;
47 #else
Is there a reason to conditionalize use of __atomic_add_fetch for
clang and continue to use __sync_add_and_fetch for gcc, rather than
using __atomic_add_fetch unconditionally?
Is __ATOMIC_RELEASE and a following FULL_MEM_BARRIER the right usage
to get the equivalent of __sync_add_and_fetch? Or should
__ATOMIC_SEQ_CST be used? Or should the latter be actively avoided?
I remember some discussion and questions in that area, about how to
implement memory_order_seq_cst on ARM processors, but wasn't paying
close attention since I don't deal with ARM much.
Andrew? I'll happily defer to you here.
------------------------------------------------------------------------------
src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp
I didn't review the stuff around os::current_stack_pointer and
os::current_frame.
------------------------------------------------------------------------------
src/hotspot/os_cpu/linux_aarch64/copy_linux_aarch64.s
162 prfum pldl1keep, [s, #-256]
I didn't review this change.
------------------------------------------------------------------------------
