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

[Doerr, Martin]

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.

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