[aarch64-port-dev ] RFR: AArch64: org.openjdk.jcstress.tests.varhandles.DekkerTest fails

[Vladimir Ivanov]

The fix looks good.

It reverts the behavior back to original (pre-8181211) when membar is 
needed, so it looks like a low risk fix.

Best regards,
Vladimir Ivanov

On 3/6/18 3:23 PM, Andrew Dinn wrote:
> Could someone please review the following patch to /shared code/ which
> fixes an AArch64 breakage that was inadvertently introduced by JDK-8181211:
> 
>    webrev: http://cr.openjdk.java.net/~adinn/8198950/webrev.00
>    JIRA: https://bugs.openjdk.java.net/browse/JDK-8198950
> 
> The patch applies to jdk/hs. It also applies cleanly to jdk/jdk10.
> 
> I would like it to be considered for inclusion in jdk10 if at all
> possible because it patches a critical error in handling of volatile
> reads that may result in incorrect memory synchronization.
> 
> The problem:
> ------------
> 
> JDK-8181211 modified generations of load nodes in library_call.c to
> allow them to float when
> 
>    i) the load is known to be from a heap address (i.e. an object)
>    ii) the object reference is know (from type info) to be non-null
>    iii) the offset for the load is known to lie within object's extent
> 
> In this case the load is created with a null control, allowing it to float.
> 
> In the case where this is a volatile load passing a null control is not
> really appropriate because the load should not be able to float.
> 
> Before 8181211 the load inherited its control and memory link from the
> leading CPUOrder memory barrier that precedes the load node. After
> 8181211, the null control passed in to the load create call may
> eventually be replaced by control flow from from an earlier node (e.g. a
> guarding ifnull node). This change does not actually allow the load to
> float because it is still dominated directly in the memory flow by the
> memory feed from the CPUOrder membar.
> 
> The problem this causes for AArch64 is that the back end generator
> attempts to replace ldr; membar_acquire sequences with ldar. In order to
> do so it has to spot that a load is actually volatile and associated
> with a trailing Acquire membar. It is currently relying on the now
> invalid assumption that the load will be dominated in both control and
> memory flow by the preceding CPUOrder membar. The generator falls back
> to generating ldr; membar_acquire.
> 
> Unfortunately, the other half of the generation scheme, which replaces
> membar_release; str; membar_volatile with an stlr instruction is still
> performed. The transformation is only valid if stlr instructions are
> paired with ldar instruction which is why the Dekker test fails.
> 
> The fix:
> --------
> 
> This patch tweaks the changes originally made to /shared code/ in
> 8181211 to ensure that a null control is only passed when a non-volatile
> load is being created (i.e. when needs_membar is false). I chose to fix
> it this way because:
> 
>    i) it models the control flow correctly
>    ii) this re-establishes the status quo ante for volatile loads which
> should, therefore be very low risk while not prejudicing the performance
> gain for non-volatile loads
>    iii) the alternative tack of modifying the graph pattern matching done
> by the AArch64 back-end generator is a relatively complex change.
> 
> alternative jdk10 fix:
> ----------------------
> 
> If this is not able to go in as a fix for jdk10 right now then it will
> still be necessary to implement an alternative fix before jdk10 is
> released otherwise AArch64 will be horribly broken.
> 
> The least complicated alternative is to switch the default setting for
> AArch64 product flag UseBarriersForVolatile to true, so that the
> transformations are disabled.
> 
> It would also be possible to rewrite the back end generator so that it
> only checks for a dominating memory feed from the CPUOrder barrier when
> trying to identify a volatile load.
> 
> Testing:
> -------
> 
> I reran jcstress tests on AArch64 and the change fixes the failing
> Dekker tests without introducing any new failures (3 opaque tests were
> failing before and after the patch). I ran all jdk tier1 jtreg tests on
> AArch64 and they passed.
> 
> As a change to shared code this also needs testing against the submit
> tree to ensure it does not break x86 code (which I am in the process of
> doing).
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>