RFR: 8322743: C2: prevent lock region elimination in OSR compilation [v2]

[Vladimir Kozlov]

On Wed, 28 Feb 2024 21:53:52 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Ok, so I did some rudimentary study of EA. And now this PR makes much more sense ;)
>> 
>> Let me summarize my understanding of the issue:
>> An object gets allocated in interpreter, and we lock on it in the interpreter.
>> OSR is triggered, the object is passed in as OSR parameter, we hold the lock.
>> The OSR control flow now looks like this:
>> 
>> 
>> StartOSR:
>>   LoadP -> load the object created in interpreter
>>   we have not_global_escape(LoadP) == false
>>   so this is correctly marked as escaping
>>   now the osr path injects into the middle of the loop
>> 
>> Loop:
>>   Phi -> merge interpreter obj and that from this compiled code
>>   we have not_global_escape(Phi) == false
>>   ...
>>   Unlock(Phi)
>>   ...
>>   check some condition, maybe return
>>   ...
>>   obj = CheckCastPP( Allocate(i.e. new Object()) )
>>   we have not_global_escape(obj) == true
>>   this is correct, the object will never escape
>>   Lock(obj)
>>   ...
>>   goto Loop
>> 
>> 
>> So if I understand this correctly, the marking in/with the ConnectionGrap is correct:
>> - The object passed in through OSR is marked as escaping.
>> - The object created locally is marked as non-escaping.
>> - The loop-phi that merges the two must therefore also be possibly escaping.
>> 
>> The question is then with the condition of Lock removal:
>> Can we remove the lock, just because its object is marked as non-escaping?
>> At first glance: obviously, because nobody else could ever have the object, and so nobody can ever lock/unlock it.
>> 
>> In the example, if we look at the Unlock node, we cannot remove it (at least at first):
>> its object is possibly escaping, because the Phi is not marked non-escaping.
>> But we can remove the Lock, since its object is non-escaping.
>> This is where the trouble starts.
>> 
>> I think it is exactly for this reason, that @vnkozlov thinks one cannot just look at the object of the individual Lock/Unlock node, but one has to look at all Lock/Unlock nodes of a BoxLock, and see if all objects are non-escaping.
>> 
>> @vnkozlov please correct me if I got something wrong ;)
>> 
>> I was trying to see what the meaning of the BoxLockNode is, but I did not find any useful documentation. Can you help me out here? Your patch assumes that all "relevant" Lock/Unlock nodes share the same BoxLockNode. Why is that the case?
>
> @eme64, @iwanowww and @dean-long please look.

> @vnkozlov title improvement suggestion: `8322743: C2: prevent elimination OSR locking region` -> `8322743: C2: prevent lock region elimination in OSR compilation`

Done

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17331#issuecomment-1971510752