Request for reviews (S): 7190310: Inlining WeakReference.get(), and hoisting $referent may lead to non-terminating loops

[John Cuthbertson]

Hi Vladimir,

This looks good to me.

I do, however, have one question. In LIRGenerator::do_UnsafeGetObject(), 
since you have moved a number of the filters into this routine, can we 
now call LirGenerator::pre_barrier() with NULL, reg, false, false, NULL) 
instead of instantiating G1UnsafeGetObjSATBBarrierStub - where reg is 
the result of get_Object_unsafe()? Moving the filters into the LIR have 
made this stub and the regular G1 pre-barrier stub very similar. That 
way we can remove the now unused G1UnsafeGetObjSATBBarrierStub.

Thanks,

JohnC

On 08/16/12 15:58, Vladimir Kozlov wrote:
> http://cr.openjdk.java.net/~kvn/7190310/webrev.02
>
> I updated changes as discussed here. I removed membar generation in C1 
> code because intrinsic nodes and not optimized by C1 GVN.
> I kept my klass analysis changes in C1 and checks move from G1 stub.
>
> Thanks,
> Vladimir
>
> Vladimir Kozlov wrote:
>> Roland Westrelin wrote:
>>>> In C2 two reads (one outside a loop and an other inside) have the 
>>>> same inputs so IGVN replace second with dominating one outside the 
>>>> loop. I don't know if C1 does it now but nothing prevents from this 
>>>> in a future when someone decide to add more optimization into C1. 
>>>> On other hand it is unsafe reads (in HIR level), I doubt it will be 
>>>> allowed to common unsafe reads.
>>>
>>> GVN in c1 operates on the HIR. Reference.get() is inlined in the HIR 
>>> as an Intrinsic instruction node. Intrinsic instructions do not 
>>> participate in GVN (something you find out by looking at the 
>>> Intrinsic definition in c1_Instruction.hpp and checking whether it 
>>> uses one of the HASHING{1,2,3} macro). So no Intrinsic instruction 
>>> will be eliminated by GVN.
>>
>> Good, I was thinking the same. Just always (before it was only for 
>> G1) generating Reference.get() intrinsic will be enough. I will 
>> remove membar from there.
>>
>>>
>>> The other thing that could access the same field would be an 
>>> UnsafeGetObject, right? GVN ignores this one as well.
>>
>> Right only these 2 cases.
>>
>>>
>>> When LIR is built, the UnsafeGetObject and the Reference.get() 
>>> Intrinsic nodes become loads. No optimizations are then applied so 
>>> no load will go away.
>>
>> I did not realized that UnsafeGetObject is like intrinsic node.
>>
>>>
>>> So to me, we are safe on the c1 side.
>>
>> Good, then I don't need membar in do_UnsafeGetObject() also.
>>
>> What do you think about my move of some checks from G1 stub into 
>> do_UnsafeGetObject()? And about klass analysis there?
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Roland.