RFR (XS): 8023472: C2 optimization breaks with G1

[Tony Printezis]

Hi all,

(Note: I'm also not familiar with C2!) I agree with Mikael: it should be 
safe to move the pre-barrier after the store / post-barrier, as long as 
you can somehow get the pre-value and there are no safepoints in 
between. However, (and if I understand it correctly) does this 
optimization only apply to the atomic operations mentioned below? If 
yes, does it actually pay off?

Tony

On 8/22/13 7:31 AM, Mikael Gerdin wrote:
>
>
> On 2013-08-22 15:48, Azeem Jiva wrote:
>> Adding the GC dev alias to get their opinion.
>>
>> -- 
>> Azeem Jiva
>> @javawithjiva
>>
>> On Aug 22, 2013, at 6:34 AM, "Doerr, Martin" <martin.doerr at sap.com
>> <mailto:martin.doerr at sap.com>> wrote:
>>
>>> Hi,
>>>
>>> we have an addon to your G1 pre-barrier change which reduces
>>> its cost.
>>> It is possible to get rid of the do_load in inline_unsafe_load_store
>>> because we always know the value which gets overwritten.
>>>
>>> In case of cmpxchg, the only value which might get overwritten
>>> is known in advance.
>>> In case of xchg, the old value is returned by the load_store.
>>> It is allowed to do the barrier after the xchg because there's
>>> no safepoint between them.
>
> I think I understand the change (not that familiar with C2).
> I agree that it should be safe to move the barrier as long as there is 
> no safepoint but I am not familiar enough with C2 to able to verify 
> that fact.
>
> But I'm not sure if the possible performance gains are worth the code 
> complexity and the confusion of having the "pre-barrier" after the 
> "post-barrier" :)
>
> /Mikael
>
>>>
>>> Please review
>>> http://cr.openjdk.java.net/~goetz/webrevs/opto_g1_barr/
>>>
>>> Best regards,
>>> Martin and Goetz
>>>
>>>
>>> -----Original Message-----
>>> From: hotspot-compiler-dev-bounces at openjdk.java.net
>>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of
>>> Vladimir Kozlov
>>> Sent: Donnerstag, 22. August 2013 02:55
>>> To: hotspot compiler
>>> Subject: Re: RFR (XS): 8023472: C2 optimization breaks with G1
>>>
>>> Thanks, Christian
>>>
>>> On 8/21/13 4:04 PM, Christian Thalinger wrote:
>>>>
>>>> On Aug 21, 2013, at 3:18 PM, Vladimir Kozlov
>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>
>>>>> http://cr.openjdk.java.net/~kvn/8023472/webrev/
>>>>>
>>>>> The load of previous value in G1 pre-barrier code (method
>>>>> GraphKit::g1_write_barrier_pre()) does not have control. It is from
>>>>> the time when G1 code was merged into Hotspot sources. I can only
>>>>> speculate that it was done to allow commoning with a preceding
>>>>> normal load if it exists and to avoid generation of 2 loads.
>>>>> This problem, I think, only affects loads from an array because a
>>>>> load from a field should be guarded by NULL check of object.
>>>>>
>>>>> The fix is to put control on this load to always generate it after
>>>>> if (marking) check. I doubt it affect performance much. If there was
>>>>> load before the value should be in cache.
>>>>
>>>> The fix looks good.  Can you at least do a refworkload run to do some
>>>> due diligence?
>>>
>>> Will do it tonight.
>>>
>>>>>
>>>>> Added regression test.
>>>>
>>>> Could you change the name of the test?  Since we moved away from
>>>> using bug ids the test name should more or less explain what it
>>>> tests.  I suppose there will be more G1CrashTest's in the future :-)
>>>
>>> It is external test. I don't want to change it much or its name.
>>>
>>> Vladimir
>>>
>>>>
>>>> -- Chris
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>

-- 
Tony Printezis | Staff Software Engineer | Twitter

@TonyPrintezis
tprintezis at twitter.com