[12] RFR(XXS): JDK-8196950: AARCH64 - Add VM flags presets for Cavium Thunder X2 CPU

[Andrew Dinn]

Hi Dmitry,

On 02/11/18 14:01, Dmitry Chuyko wrote:
> On 11/1/18 12:47 PM, Andrew Dinn wrote:
>> On 31/10/18 17:40, Dmitry Chuyko wrote:
>>> Please review small defaults correction.
>>>
>>> rfe: https://bugs.openjdk.java.net/browse/JDK-8196950
>>> code:
>>>
>>> diff -r 16950b2eaebf src/hotspot/cpu/aarch64/vm_version_aarch64.cpp
>>> --- a/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp    Tue Oct 30
>>> 09:13:00 2018 -0400
>>> +++ b/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp    Wed Oct 31
>>> 20:33:41 2018 +0300
>>> @@ -219,7 +219,7 @@
>>>       }
>>>   #ifdef COMPILER2
>>>       if (FLAG_IS_DEFAULT(UseFPUForSpilling)) {
>>> -      FLAG_SET_DEFAULT(UseFPUForSpilling, true);
>>> +      FLAG_SET_DEFAULT(UseFPUForSpilling, false);
>>>       }
>>>   #endif
>>>     }
>>>
>>> Having "-XX:+AvoidUnalignedAccesses -XX:+UseSIMDForMemoryOps
>>> -XX:-UseFPUForSpilling" as a default works (in general) better for
>>> SPECjvm and SPECjbb.
>> That's fine as a goal. However, I don't think this is the right patch.
>>
>> Flag UseFPUForSpilling is defined in
>> src/hotspot/share/opto/c2_globals.hpp as follows:
>>
>>    product(bool, UseFPUForSpilling, false,
>>     \
>>            "Spill integer registers to FPU instead of stack when
>> possible")  \
>>
>> In other words the global default is already false. Indeed, that is the
>> the default used for the other AArch64 variants.
>>
>> So, the correct patch is to remove the whole if clause. Of course, you
>> should also remove the surrounding ifdef COMPILER2 endif that brackets
>> it.
> 
> That's an option. Though "default" (false) and "false" are not exactly
> the same, in the sense of explicitly expressing the difference in
> benchmarks score, we'll have history for that.

I am afraid don't know what that is supposed to mean. Can you try
explaining it again, perhaps in more detail?

> Webrev with updated patch:
> http://cr.openjdk.java.net/~dchuyko/8198294/webrev.00/
Well, the patch now looks good to me. However, I'm not sure if your
comment means I need to revise that view. My belief is that:

i) this is the better way to patch the code (because it removes
unnecessary code)

ii) the result will be functionally /identical/ to that obtained using
the patch you initially suggested

If your statement above is supposed to counter the second of those
beliefs is wrong I'd be glad to be corrected.

regards,


Andrew Dinn
-----------