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

[Christian Thalinger]

On Aug 17, 2012, at 11:59 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> 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

Even better.  -- Chris

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