RFR(XS): 8230434: [C1, C2] Release barrier for volatile field stores in constructors implemented inconsistently

[Doerr, Martin]

Hi Aleksey,

thanks for looking at this.

Sure, I can also clean up coding style:
http://cr.openjdk.java.net/~mdoerr/8230434_membar_volatile_field_constructor/webrev.01/

Comments are improved and the order changed according to the implementation.

Best regards,
Martin


> -----Original Message-----
> From: Aleksey Shipilev <shade at redhat.com>
> Sent: Dienstag, 3. September 2019 10:15
> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: RFR(XS): 8230434: [C1, C2] Release barrier for volatile field stores
> in constructors implemented inconsistently
> 
> On 9/2/19 6:56 PM, Doerr, Martin wrote:
> > As an additional cleanup, I'd like to use it consistently between C1 and C2
> compiler:
> >
> http://cr.openjdk.java.net/~mdoerr/8230434_membar_volatile_field_constr
> uctor/webrev.00/
> 
> The idea looks good.
> 
> Patch nits:
> 
> *) In parse1.cpp, the comment section still mentions PPC64 and uses the old
> condition ordering. Why
> don't we keep ordering intact and replace the mentions of PPC64? Also, do
> we need to change bracing
> style? I am thinking this:
> 
>    if (method()->is_initializer() &&
>         (wrote_final() ||
>           (support_IRIW_for_not_multiple_copy_atomic_cpu &&
> wrote_volatile()) ||
>           (AlwaysSafeConstructors && wrote_fields()))) {
> 
> *) c1_GraphBuilder.cpp would probably need to match the style:
> 
>    // The conditions for a memory barrier are described in Parse::do_exits().
>    bool need_mem_bar = false;
>    if (method()->name() == ciSymbol::object_initializer_name() &&
>         (scope()->wrote_final() ||
>           (support_IRIW_for_not_multiple_copy_atomic_cpu && scope()-
> >wrote_volatile()) ||
>           (AlwaysSafeConstructors && scope()->wrote_fields()))) {
> 
> Or, if we don't want to do cleanups, just replace PPC64_ONLY ->
> support_IRIW_for_not_multiple_copy_atomic_cpu, without changing
> anything else code-wise.
> 
> --
> Thanks,
> -Aleksey