AARCH64: 8064611: Changes to HotSpot shared code

[Volker Simonis]

Hi,

I think Goetz answered to the remaining questions a few days ago:

http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2014-November/001855.html
http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-November/016199.html

but for some reason his mail doesn't appear in this mail thread.

As he wrote, the release store into the card table in graphKit.cpp
isn't needed and we've just removed in in our internal version a few
weeks ago as well.

The first one in memnode.hpp is indeed only needed on IA64 so your
solution with AARCH64_ONLY is OK for us.

Regards,
Volker

On Fri, Nov 21, 2014 at 7:06 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> 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.
>>
>