[aarch64-port-dev ] RFR: AArch64: incorrect code generation for StoreCM

[Zhongwei Yao]

Hi, Andrew,
Thanks to your patch for fixing this!

I find a typo in the TestVolatiles.java file: it uses "CMSCondCardMark" in checkstore() and checkcas() but the testType is past as "CMSCondMark". It leads to "CMSCondCardMark" case not tested. 

And the condition in aarch64.ad:2871 (patched)
  UseConcMarkSweepGC || !UseCondCardMark
should be "UseConcMarkSweepGC && !UseCondCardMark", right? (It could be catched if the previous typo is fixed).

I'm thinking whether it is OK to replace "stlr; dmb ishst; strb" with "stlr; strlb" when we using CMS without conditional card marking. What do you think?

--
Best regards,
Zhongwei

________________________________________
From: aarch64-port-dev <aarch64-port-dev-bounces at openjdk.java.net> on behalf of Andrew Dinn <adinn at redhat.com>
Sent: Tuesday, July 3, 2018 6:21 PM
To: Andrew Haley; aarch64-port-dev at openjdk.java.net; hotspot compiler
Subject: Re: [aarch64-port-dev ] RFR: AArch64: incorrect code generation for StoreCM

On 03/07/18 11:16, Andrew Dinn wrote:
> Aaaargh -- minor fix applied below

And another ...

> On 03/07/18 11:11, Andrew Dinn wrote:
>> On 03/07/18 10:26, aph wrote:
>>> On 07/03/2018 09:57 AM, Andrew Dinn wrote:
>>>> The correct generated sequences should be
>>>>
>>>>   str ; dmb ishst ; strb
>>>>
>>>> and
>>>>
>>>>   dmb ish ; stlr; dmb ishst ; strb ; dmb ish
>>>
>>> What is the leading  DMB ISH  supposed to do here?  The STRB can't move and
>>> the STLR is enough for a volatile store.
>> Oops, as if this is not confusing enough ... Sorry, that was explained
>> all wrong. Let me try again.
>>
>> For a volatile store with gc config XX:+UseConcMarkSeepGC
>> -XX:-UseCondCardMark the back end used to generate
>>
>>   stlr   # when -XX:-UseBarriersForVolatile
>>   strb
>>
>> and
>>
>>   dmb ish   # when -XX:+UseBarriersForVolatile
>>   str
>>   dmb ishst
>>   strb
>>   dmb ish
>>
>> for cases -XX:-UseBarriersForVolatile and -XX:+UseBarriersForVolatile,
>> respectively. The patch corrects the former case to
>>
>>   stlr   # when -XX:-UseBarriersForVolatile
>>   dmb ishst
>>   strb
>>
>> The latter case is already correct.
>>
>> Similarly, for a CAS the generated code used to look like
>>
>>   cmpxchw_acq  # when -XX:-UseBarriersForVolatile
>>   strb
>>
>> and
>>
> ERROR:
>>   dmb ish    # when -XX:-UseBarriersForVolatile
> FIX:
>    dmb ish    # when -XX:+UseBarriersForVolatile
>>   cmpxchw
>>   dmb ishst
>>   strb
>>   dmb ish
>>
ERROR
>> where cmpxch_acq uses an ldar to load and store the CASed field value
FIX
  where cmpxch_acq uses an ldar to load and stlr to store the CASed
field value
>> and cmpxchw uses a plain ldr and str
>>
>> The patch corrects the former case to
>>
>>   cmpxch_acq  # when -XX:-UseBarriersForVolatile
>>   dmb ishst
>>   strb
>>
>> The latter case is already correct.
>>
>> Apologies for muddying already obscure waters.
>>
>> regards,
>>
>>
>>
>> Andrew Dinn
>> -----------
>> Senior Principal Software Engineer
>> Red Hat UK Ltd
>> Registered in England and Wales under Company Registration No. 03798903
>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>

--
regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander