RFR: 8321371: SpinPause() not implemented for bsd_aarch64/macOS

[Evgeny Astigeevich]

On Wed, 20 Dec 2023 12:17:32 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> @fisk I don't mind to use `yield`.
>> 
>> `isb` appeared as a result of real customer cases and consultations with hardware engineers. It was a real problem to write a micro-benchmark. I think it's still a problem.
>> We are aware that different implementations of AArch64 might need different spin pause implementations. This is why we provide options which instructions to use: `yield`, `nop` or `isb`.
>> 
>> I think hardcoding `yield` is not a good idea. We should use the current implemented approach where `yield` is selected. If the current approach does not work because it is based on a generated stub in CodeCache. Let's modify it to get code in place instead of stub.
>> 
>> @fbredber @fisk 
>> What do  you think about the following solution:
>> 
>> In `src/hotspot/cpu/aarch64/spin_wait_aarch64.hpp` we add `DEFAULT` to `SpinWait::Inst` which is set to `YIELD`. We also add a template function `exec<SpinWait::Instr>()` and its implementation `exec<SpinWait::YIELD>()`
>> 
>> 
>> class SpinWait {
>> public:
>>   enum Inst {
>>     NONE = -1,
>>     NOP,
>>     ISB,
>>     YIELD,
>>     DEFAULT = YIELD
>>   };
>> ...
>> };
>> 
>> template<SpinWait::Inst inst> void exec();
>> 
>> template<> exec<SpinWait::YIELD>() {
>>   asm volatile("yield" : : : "memory");
>> }
>> 
>> 
>> In `src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp`:
>> 
>> 
>> extern "C" {
>>   int SpinPause() {
>>     exec<SpinWait::DEFAULT>();
>>     return 1;
>>   }
>> 
>> 
>> To enable onSpinWait intrinsic (this or a separate PR) you will need to set `yield` as default in `src/hotspot/cpu/aarch64/globals_aarch64.hpp`
>> 
>>   product(ccstr, OnSpinWaitInst, "yield", DIAGNOSTIC,                    \
>>           "The instruction to use to implement "                        \
>>           "java.lang.Thread.onSpinWait()."                              \
>>           "Options: none, nop, isb, yield.")                            \
>> 
>> You will also need to update tests:
>> - `test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.java`
>> - `test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitNoneAArch64.java`
>
> @eastig @fisk 
> 
> I've now rewritten `SpinPause()` so you can use the `OnSpinWaitInst` option to choose between `none`, `nop`, `isb` and `yield`.
> I choose not to change the default from `none` to `yield` in this PR. For that I've created [JDK-8322535](https://bugs.openjdk.org/browse/JDK-8322535).

@fbredber 
The assembly code looks good to me.
Out of curiosity, why not to use a regular `switch`? Why is the manually written assembly better?
What if it looked like the following:

class SpinWait {
public:
   static void exec(Inst inst) {
      switch (inst) {
        case NOP:
            asm volatile();
            break;
        case ISB:
            asm volatile();
            break;
         case YIELD:
            asm volatile();
            break;
        default:
      }
   }
...
};

extern "C" {
  int SpinPause() {
    SpinWait::exec(VM_Version::spin_wait_desc().inst());
    return 1;
  }

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

PR Comment: https://git.openjdk.org/jdk/pull/16994#issuecomment-1865147655