Question about tearing and buffered primitive classes

[Tobias Hartmann]

Hi Nick,

Thanks for looking into this!

On 30.03.21 12:41, Nick Gasson wrote:
> However based on my reading of the description in JDK-8237741 [1] this
> should only be necessary when the object is passed by invisible
> reference, not when it's actually flattened.  In Tobias' fix for
> JDK-8238780 [2] a store-store barrier was added to C2's
> InlineTypeBaseNode::buffer() but I can't see where the equivalent
> "buffering" happens in C1.

C2 does aggressive scalarization and therefore needs to buffer at various places when a reference is
required. C1 buffers only after reading from a flat representation, for example in
GraphBuilder::access_field (see NewInlineTypeInstance -> copy_inline_content).

That said, I'm wondering if a store-store barrier is needed there as well.

> Furthermore I don't understand why we can't instead rely on the barrier
> that gets added before returning from a constructor that writes final
> fields?  AIUI a primitive class's fields are always final and `this'
> can't escape its constructor so the effect should be the same as putting
> a barrier after each intermediate allocation inside the constructor.
> 
> GraphBuilder::withfield() calls set_wrote_final() which is used for this
> purpose *but* in GraphBuilder::method_return() we don't set need_mem_bar
> because method()->is_object_constructor() is false for primitive class
> constructors.  So a different patch that also fixes this test case is:

Yes, I think this is the correct fix:

> diff --git a/src/hotspot/share/c1/c1_GraphBuilder.cpp b/src/hotspot/share/c1/c1_GraphBuilder.cpp
> index ead767f596d7..ac7e4ce5f06d 100644
> --- a/src/hotspot/share/c1/c1_GraphBuilder.cpp
> +++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp
> @@ -1617,7 +1617,7 @@ void GraphBuilder::method_return(Value x, bool ignore_return) {
> 
>    // The conditions for a memory barrier are described in Parse::do_exits().
>    bool need_mem_bar = false;
> -  if (method()->is_object_constructor() &&
> +  if ((method()->is_object_constructor() || method()->is_static_init_factory()) &&
>         (scope()->wrote_final() ||
>           (AlwaysSafeConstructors && scope()->wrote_fields()) ||
>           (support_IRIW_for_not_multiple_copy_atomic_cpu && scope()->wrote_volatile()))) {

The is_static_init_factory check is simply missing there.

Best regards,
Tobias