AARCH64: 8064611: Changes to HotSpot shared code

[David Holmes]

On 28/11/2014 11:41 PM, Volker Simonis wrote:
> 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.

Has anyone from hotspot team confirmed that analysis? I haven't seen 
anything to that effect and I'm wary of making this kind of change to 
shared code.

Thanks,
David

> 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.
>>>
>>