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

[Vladimir Kozlov]

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