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

[Tobias Hartmann]

Hi Andrew,

the fix looks reasonable to me. Do I understand correctly, that the impact on code generation on
other platforms should be minimal because even without a control edge, volatile loads can not float
above the CPUOrder memory barrier? Probably, final code isn't even affected at all on x86.

I'm not sure about inclusion into JDK 10 but I think this fix is low risk.

Thanks,
Tobias


On 06.03.2018 13:23, 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
>