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

[Mikael Gerdin]

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