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

[Vladimir Kozlov]

Thank you, John

Vladimir

John Cuthbertson wrote:
> Hi Vladimir,
> 
> Looks good.
> 
> JohnC
> 
> On 08/17/12 11:59, Vladimir Kozlov wrote:
>> Yes, I do like cleanup unused code :)
>>
>> I did suggested changes and verified generated asm code. Bug's tests 
>> passed. Here is updated (I hope final) webrev with C1 
>> G1UnsafeGetObjSATBBarrierStub gone on all platforms:
>>
>> http://cr.openjdk.java.net/~kvn/7190310/webrev.03
>>
>> Thanks,
>> Vladimir
>>
>> John Cuthbertson wrote:
>>> 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.
>>>
>