RFR: 8325821: [REDO] use "dmb.ishst+dmb.ishld" for release barrier [v6]

[Aleksey Shipilev]

On Thu, 30 May 2024 07:45:31 GMT, kuaiwei <duke at openjdk.org> wrote:

>> he origin patch for https://bugs.openjdk.org/browse/JDK-8324186 has 2 issues:
>> 1 It show regression in some platform, like Apple silicon in mac os
>> 2 Can not handle instruction sequence like "dmb.ishld; dmb.ishst; dmb.ishld; dmb.ishld"
>> 
>> It can be fixed by:
>> 1 Enable AlwaysMergeDMB by default, only disable it in architecture we can see performance improvement (N1 or N2)
>> 2 Check the special pattern and merge the subsequent dmb.
>> 
>> It also fix a bug when code buffer is expanding, st/ld/dmb can not be merged. I added unit tests for these.
>> 
>> This patch still has a unhandled case. Insts like "dmb.ishld; dmb.ishst; dmb.ish", it will merge the last 2 instructions and can not merge all three. Because when emitting dmb.ish, if merge all previous dmbs, the code buffer will shrink the size. I think it may break some resumption and think it's not a common pattern.
>> 
>> In previous PR https://github.com/openjdk/jdk/pull/18467 , I tried an implementation to use state machine for merging. But it looks risky to pending instruction during emitting.
>
> kuaiwei has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add comment in aarch64.ad

src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 153:

> 151:     Assembler::bind(L);
> 152:     code()->clear_last_insn();
> 153:     code()->set_last_label(pc());

OK, so we have added `_last_label` to shared code in `codeBuffer`, but only update it in aarch64. This would be surprising for other platforms. On the other hand, this is what we already do with `_last_insn` -- only implementing it for specific platforms. Probably fine, but it would be nice to strengthen this with asserts, maybe in separate PR.

test/hotspot/gtest/aarch64/test_assembler_aarch64.cpp line 211:

> 209: constexpr uint32_t test_encode_dmb_ld = 0xd50339bf;
> 210: constexpr uint32_t test_encode_dmb_st = 0xd5033abf;
> 211: constexpr uint32_t test_encode_dmb    = 0xd5033bbf;

Can you maybe move these to the top, and use these constants across the test? You would not need the comments like `0xd5033abf, // dmb.ishst` then.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19278#discussion_r1622251263
PR Review Comment: https://git.openjdk.org/jdk/pull/19278#discussion_r1622170658