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

[John Cuthbertson]

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