RFR: 8300148: Consider using a StoreStore barrier instead of Release barrier on ctor exit [v9]

[Aleksey Shipilev]

On Thu, 4 Apr 2024 21:56:40 GMT, Joshua Cao <duke at openjdk.org> wrote:

>> The [JSR 133 cookbook](https://gee.cs.oswego.edu/dl/jmm/cookbook.html) has long recommended using a `StoreStore` barrier at the end of constructors that write to final fields. `StoreStore` barriers are much cheaper on arm machines as shown in benchmarks in this issue as well as https://bugs.openjdk.org/browse/JDK-8324186.
>> 
>> This change does not improve the case for constructors for objects with volatile fields because [MemBarRelease is emitted for volatile stores](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp#L211). This is demonstrated in test case `classWithVolatile`, where this patch does not impact the IR.
>> 
>> I had to modify some code around escape analysis to make sure there are no regressions in eliminating allocations and `StoreStore`'s. The [current handling of StoreStore's in escape analysis](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/escape.cpp#L2590) makes the assumption that the barriers input is a `Proj` to an `Allocate` ([example](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/library_call.cpp#L1553)). This is contrary to the barriers in the end of the constructor where there the barrier directly takes in an `Allocate` without an in between `Proj`. I opted to instead eliminate `StoreStore`s in GVN, exactly how `MemBarRelease` is handled.
>> 
>> I had to add [checks for StoreStore in macro.cpp](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/macro.cpp#L636), or else we fail some [cases for reducing allocation merges](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/AllocationMergesTests.java#L1233-L1256).
>> 
>> Passes hotspot tier1 locally on a Linux machine.
>> 
>> ### Benchmarks
>> 
>> Running Renaissance ParNnemonics on an Amazon Graviton (arm) instance.
>> 
>> Baseline:
>> 
>> Result "org.renaissance.jdk.streams.JmhParMnemonics.run":
>>   N = 25
>>   mean =   3309.611 ±(99.9%) 86.699 ms/op
>> 
>>   Histogram, ms/op:
>>     [3000.000, 3050.000) = 0
>>     [3050.000, 3100.000) = 4
>>     [3100.000, 3150.000) = 1
>>     [3150.000, 3200.000) = 0
>>     [3200.000, 3250.000) = 0
>>     [3250.000, 3300.000) = 0
>>     [3300.000, 3350.000) = 9
>>     [3350.000, 3400.000) = 6
>>     [3400.000, 3450.000) = 5
>> 
>>   Percentiles, ms/op:
>>       p(0...
>
> Joshua Cao has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Guard everything by feature flag
>  - Revert "Statistics for barriers generated/eliminated"
>    
>    This reverts commit 33d23635048afd3a1b40ae91e6fadf577742fa4f.

I am okay with this, but it needs more eyes.

src/hotspot/share/opto/c2_globals.hpp line 794:

> 792:                                                                             \
> 793:   product(bool, UseStoreStoreForCtor, true, DIAGNOSTIC,                     \
> 794:           "Use StoreStore barrier instead of Release barrier at the end of" \

Should be a space after "of ", these lines are just concatenated.

src/hotspot/share/opto/macro.cpp line 640:

> 638:                   use->Opcode() == Op_MemBarRelease ||
> 639:                   (UseStoreStoreForCtor &&
> 640:                    use->Opcode() == Op_MemBarStoreStore))) {

Suggestion:

                  (UseStoreStoreForCtor && use->Opcode() == Op_MemBarStoreStore))) {

src/hotspot/share/opto/memnode.cpp line 3428:

> 3426:       }
> 3427:     } else if (opc == Op_MemBarRelease ||
> 3428:                (UseStoreStoreForCtor && opc == Op_MemBarStoreStore)) {

Suggestion:

    } else if (opc == Op_MemBarRelease || (UseStoreStoreForCtor && opc == Op_MemBarStoreStore)) {

src/hotspot/share/opto/parse1.cpp line 1020:

> 1018:          (support_IRIW_for_not_multiple_copy_atomic_cpu && wrote_volatile()))) {
> 1019:     _exits.insert_mem_bar(UseStoreStoreForCtor ? Op_MemBarStoreStore
> 1020:                                                : Op_MemBarRelease,

Suggestion:

    _exits.insert_mem_bar(UseStoreStoreForCtor ? Op_MemBarStoreStore : Op_MemBarRelease,

src/hotspot/share/opto/stringopts.cpp line 2014:

> 2012:     assert(AllocateNode::Ideal_allocation(result) != nullptr, "should be newly allocated");
> 2013:     kit.insert_mem_bar(
> 2014:         UseStoreStoreForCtor ? Op_MemBarStoreStore : Op_MemBarRelease, result);

Suggestion:

    kit.insert_mem_bar(UseStoreStoreForCtor ? Op_MemBarStoreStore : Op_MemBarRelease, result);

-------------

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18505#pullrequestreview-1983748538
PR Review Comment: https://git.openjdk.org/jdk/pull/18505#discussion_r1554020869
PR Review Comment: https://git.openjdk.org/jdk/pull/18505#discussion_r1554021686
PR Review Comment: https://git.openjdk.org/jdk/pull/18505#discussion_r1554025465
PR Review Comment: https://git.openjdk.org/jdk/pull/18505#discussion_r1554025863
PR Review Comment: https://git.openjdk.org/jdk/pull/18505#discussion_r1554026083