RFR: [11u] 8217368: AArch64: C2 recursive stack locking optimisation not triggered

[Andrew Haley]

On 8/22/19 5:06 PM, Andrew Dinn wrote:
> Could I please have a review for this backport to jdk11u-dev.
> 
>   webrev: http://cr.openjdk.java.net/~adinn/8217368-jdk11u/webrev.00/
> 
>   upstream patch: http://hg.openjdk.java.net/jdk/jdk/rev/cb43e14dc68b
> 
> The original patch does not apply cleanly for two reasons:
> 
> 1) The upstream change was applied on top of changes included from
> 
>   JDK-8210381: Obsolete EmitSync
> 
> which has not been backported (for good reason -- it affects generic and
> arch-specific code). Where appropriate the modified patch interleaves
> the upstream changes into branches protected by tests of EmitSync that
> are still present in jdk11u but no longer present upstream.
> 
> 2) The upstream change includes a modification to LockUnlock.java in the
> micro test suite extending that test so it exercises this patch. This
> part of the change set has been omitted because the micro test suite has
> not been backported to jdk11u (again for good reason).

This is tricky to review because the actual bug fix is swamped by the change
from expanded inline code to do the three CASes. As far as I can tell the
bug fix itself is just a few instructions:

diff --git a/src/hotspot/cpu/aarch64/aarch64.ad b/src/hotspot/cpu/aarch64/aarch64.ad
--- a/src/hotspot/cpu/aarch64/aarch64.ad
+++ b/src/hotspot/cpu/aarch64/aarch64.ad
@@ -3340,39 +3340,23 @@
     // Handle existing monitor
     if ((EmitSync & 0x02) == 0) {
       __ tbnz(disp_hdr, exact_log2(markOopDesc::monitor_value), object_has_monitor);
     }

-    // Set displaced_header to be (markOop of object | UNLOCK_VALUE).
-    __ orr(disp_hdr, disp_hdr, markOopDesc::unlocked_value);
-
-    // Load Compare Value application register.
+    // Set tmp to be (markOop of object | UNLOCK_VALUE).
+    __ orr(tmp, disp_hdr, markOopDesc::unlocked_value);

     // Initialize the box. (Must happen before we update the object mark!)
-    __ str(disp_hdr, Address(box, BasicLock::displaced_header_offset_in_bytes()));
-
+    __ str(tmp, Address(box, BasicLock::displaced_header_offset_in_bytes()));
+
     assert(oopDesc::mark_offset_in_bytes() == 0, "offset of _mark is not 0");

@@ -3389,38 +3373,21 @@
       __ bind(object_has_monitor);
       // The object's monitor m is unlocked iff m->owner == NULL,
       // otherwise m->owner may contain a thread or a stack address.
       //
       // Try to CAS m->owner from NULL to current thread.
       __ add(tmp, disp_hdr, (ObjectMonitor::owner_offset_in_bytes()-markOopDesc::monitor_value));
-      __ mov(disp_hdr, zr);

       // Store a non-null value into the box to avoid looking like a re-entrant
       // lock. The fast-path monitor unlock code checks for

Am I right?

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