RFR: 8224974: Implement JEP 352

[Aleksey Shipilev]

On 7/30/19 5:04 PM, Andrew Dinn wrote:
> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
> for the implementation JIRA has been rebased to apply to the current
> tree. Is it now ok to push this change set?
> 
> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
Is it okay to have a late code review? Here it goes:

=== General:

So if pre wbsync is no-op, why do we need to handle it everywhere? We seem to be falling through all
the way to the stub to do nothing there, maybe we should instead cut much earlier, e.g. when
inlining Unsafe.writeBackPresync0? Would it be better to not emit CacheWBPreSync at all?

=== General:

IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? develop and ASSERT must be
the substitutes for this. See some discussion here: https://bugs.openjdk.java.net/browse/JDK-8183287

=== src/hotspot/cpu/aarch64/aarch64.ad
    src/hotspot/cpu/x86/x86.ad

This should probably be just "!VM_Version...". Braces around the statement would not hurt either.

2196       if (VM_Version::supports_data_cache_line_flush() == false)
2197         ret_value = false;

=== src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
    src/hotspot/cpu/x86/macroAssembler_x86.cpp

Braces style mismatch, should be on the same line, as in the rest of the file:

5837 void MacroAssembler::cache_wb(Address line)
5838 {

5856 void MacroAssembler::cache_wbsync(bool is_pre)
5857 {

=== src/hotspot/cpu/x86/assembler_x86.cpp

It feels like these comments are redundant, especially L8630 and L8646 which mention magic values
"6" and "7", not present in the code:

8624   // instruction prefix is 0x66

8627   // opcode family is 0x0f 0xAE

8630   // extended opcode byte is 7 == rdi

8640   // instruction prefix is 0x66

8643   // opcode family is 0x0f 0xAE

8646   // extended opcode byte is 6 == rsi


=== src/hotspot/cpu/x86/macroAssembler_x86.cpp

These comments feel redundant too. Well, maybe they should instead talk about the choices the
subsequent code makes.

9915   // pick the correct implementation

9936   // pick the correct implementation


=== src/hotspot/cpu/x86/macroAssembler_x86.cpp
   src/hotspot/cpu/x86/vm_version_x86.hpp

The docs say "The CLFLUSH instruction was introduced with the SSE2 extensions; however, because it
has its own CPUID feature flag, it can be implemented in IA-32 processors that do not include the
SSE2 extensions. Also, detecting the presence of the SSE2 extensions with the CPUID instruction does
not guarantee that the CLFLUSH instruction is implemented in the processor."

Yet, we have:

9910   // 64 bit cpus always support clflush
9911   assert(VM_Version::supports_clflush(), "should not reach here on 32-bit");

 505 #ifdef _LP64
 506     // cpflush is always available on x86_64
 507     result |= CPU_FLUSH;
 508 #else
 509     if (_cpuid_info.std_cpuid1_edx.bits.clflush != 0)
 510       result |= CPU_FLUSH;
 511 #endif

 967   // 64 bit cpus always support clflush which writes back and evicts
 968   // on 32 bit cpus support is recorded via a feature flag

 980   static bool supports_clflush() { return true; }

I think the assert should just say "clflush should be available", and clflush cpu flag detected for
64-bits too?


=== src/hotspot/cpu/x86/macroAssembler_x86.hpp

Accidental camelCase, while hotspot code expects snake_case:

1794   void cache_wbsync(bool isPre);


=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

Any reason to avoid inline here, e.g. "__ cache_wb(Address(src, 0))"?

2921     const Address line(src, 0);
2922     __ cache_wb(line);


=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

Is it really "cmpl" here, not "cmpb"? I think aarch64 code tests for byte.

2942     __ cmpl(is_pre, 0);


=== src/hotspot/share/classfile/vmSymbols.hpp

Excess space before "jdk_internal..." here:

1096   do_intrinsic(_writebackPostSync0,        jdk_internal_misc_Unsafe,
writebackPostSync0_name, void_method_signature , F_RN)   \


=== src/hotspot/share/opto/c2compiler.cpp

Why inject new cases here, instead of at the end of switch? Saves sudden "break":

 578     break;
 579   case vmIntrinsics::_writeback0:
 580     if (!Matcher::match_rule_supported(Op_CacheWB)) return false;
 581     break;
 582   case vmIntrinsics::_writebackPreSync0:
 583     if (!Matcher::match_rule_supported(Op_CacheWBPreSync)) return false;
 584     break;
 585   case vmIntrinsics::_writebackPostSync0:
 586     if (!Matcher::match_rule_supported(Op_CacheWBPostSync)) return false;
 587     break;

=== src/hotspot/share/opto/library_call.cpp

Accidental camelCase here, should be snake_case:

  257   bool inline_unsafe_writebackSync0(bool isPre);

 2870 bool LibraryCallKit::inline_unsafe_writebackSync0(bool isPre) {


=== src/hotspot/share/prims/unsafe.cpp

Odd indenting near "CC":

1122     {CC "writeback0",         CC "(" "J" ")V",           FN_PTR(Unsafe_WriteBack0)},
1123     {CC "writebackPreSync0",     CC "()V",               FN_PTR(Unsafe_WriteBackPreSync0)},
1124     {CC "writebackPostSync0",     CC "()V",              FN_PTR(Unsafe_WriteBackPostSync0)},

camelCase:
  464 static void doWriteBackSync0(bool isPre)


=== src/hotspot/share/prims/unsafe.cpp

Do we really need this function pointer mess?

 457   void (*wb)(void *);
 458   void *a = addr_from_java(line);
 459   wb = (void (*)(void *)) StubRoutines::data_cache_writeback();
 460   assert(wb != NULL, "generate writeback stub!");
 461   (*wb)(a);

Seems easier to:

 assert(StubRoutines::data_cache_writeback() != NULL, "sanity");
 StubRoutines::data_cache_writeback()(addr_from_java(line));


Thanks,
-Aleksey