[aarch64-port-dev ] AARCH64: 8064611: Changes to HotSpot shared code

[Vladimir Kozlov]

Hi Andrew,

What is the status of these changes?
I hope you are not waiting some our response.

Regards,
Vladimir

On 11/21/14 10:06 AM, Vladimir Kozlov wrote:
> On 11/21/14 9:33 AM, Andrew Haley wrote:
>> On 11/20/2014 06:13 PM, Andrew Haley wrote:
>>> On 11/20/2014 06:05 PM, Vladimir Kozlov wrote:
>>>> I based the name on your comment:
>>>>
>>>> +  // AArch64 uses store release (which does everything we need to keep
>>>> +  // the machine in order) but we still need a compiler barrier here.
>>>
>>> Ah.  Okay, I'll have to think of a good name for it, then.
>>>
>>>> You can name it as you like. Our main suggestion is to use such Boolean
>>>> constant and normal if() statements instead of ifdef AARCH64 and
>>>> AARCH64_ONLY/NOT_AARCH64 macros in C2 code (src/share/vm/opto/* files).
>>>>
>>>> We already do similar things for PPC64 port which sets
>>>> support_IRIW_for_* constant.
>>>
>>> Okay,
>>
>> I've done something similar but more useful.  I've added an
>> experimental flag: UseBarriersForVolatile.  This defaults to true for
>> all targets, but we can override it in the back end.  That gives me
>> the chance to do some benchmarking on various AArch64 targets to see
>> which ones benefit from the new load acquire/store release
>> instructions.
>
> Okay.
>
>>
>> I have kept AARCH64_ONLY for two hunks:
>>
>> --- old/src/share/vm/opto/memnode.hpp    2014-11-21 12:09:22.766963837
>> -0500
>> +++ new/src/share/vm/opto/memnode.hpp    2014-11-21 12:09:22.546983320
>> -0500
>> @@ -503,6 +503,10 @@
>>     // Conservatively release stores of object references in order to
>>     // ensure visibility of object initialization.
>>     static inline MemOrd release_if_reference(const BasicType t) {
>> +    // AArch64 doesn't need a release store here because object
>> +    // initialization contains the necessary barriers.
>> +    AARCH64_ONLY(return unordered);
>> +
>>       const MemOrd mo = (t == T_ARRAY ||
>>                          t == T_ADDRESS || // Might be the address of
>> an object reference (`boxing').
>>                          t == T_OBJECT) ? release : unordered;
>
> This could be needed for ppc64 too, not only for IA64.
>
>>
>> --- old/src/share/vm/opto/graphKit.cpp    2014-11-21
>> 12:09:20.017207376 -0500
>> +++ new/src/share/vm/opto/graphKit.cpp    2014-11-21
>> 12:09:19.787227745 -0500
>> @@ -3813,7 +3813,8 @@
>>
>>     // Smash zero into card
>>     if( !UseConcMarkSweepGC ) {
>> -    __ store(__ ctrl(), card_adr, zero, bt, adr_type, MemNode::release);
>> +    __ store(__ ctrl(), card_adr, zero, bt, adr_type,
>> +             NOT_AARCH64(MemNode::release)
>> AARCH64_ONLY(MemNode::unordered));
>>     } else {
>>       // Specialized path for CM store barrier
>>       __ storeCM(__ ctrl(), card_adr, zero, oop_store, adr_idx, bt,
>> adr_type);
>
> Looks like PPC64 needs that. In ppc.ad:
>
>    // Use release_store for card-marking to ensure that previous
>    // oop-stores are visible before the card-mark change.
>    enc_class enc_cms_card_mark(memory mem, iRegLdst releaseFieldAddr) %{
>
>>
>> The first hunk is only required by IA64 as far as I am aware, but I
>> am nervous about making it IA64_ONLY.  The second hunk is a release
>> node which is not as far as I am aware required by any target, and
>> should simply be removed.
>>
>> This isn't a RFA because it's not tested yet, but what do you think?
>
> Since it affects ppc64 and ia64 we need to ask Goetz and Co.
> I would suggest to put both these places under platform specific
> flags/bool constant.
>
> Thanks,
> Vladimir
>
>>
>> Andrew.
>>