RFR (S): 8003135: HotSpot inlines and hoists the Thread.currentThread().isInterrupted() out of the loop

[Vladimir Ivanov]

Vladimir K.,

Thanks for the review!

 > You don't need second region node, use one.
Can you elaborate on this a little?
Do you suggest to remove slow_region node? What should I use to point to 
slow path (for example, in generate_slow_guard(bol_thr, slow_call))?

 > If you rearrange code to generate slow call first and the load on 
fast path last you may not need to preserve init_mem.
When considering moving slow path construction up, I was concerned about 
the case where there's no slow path:
   if (stopped()) {
     // There is no slow path.
     result_rgn->init_req(slow_result_path, top());
     result_val->init_req(slow_result_path, top());
   } else {

Is it worth retaining?

Best regards,
Vladimir Ivanov

On 12/14/12 2:05 AM, Vladimir Kozlov wrote:
> John, the barrier is before enum {} in simple fix.
>
> But I also prefer fast code.
>
> Vladimir I.,
>
> You don't need second region node, use one.
>
> If you rearrange code to generate slow call first and the load on fast
> path last you may not need to preserve init_mem.
>
> Thanks,
> Vladimir K.
>
> On 12/13/12 2:31 PM, John Rose wrote:
>> On Dec 13, 2012, at 3:02 PM, Vladimir Ivanov wrote:
>>
>>> http://cr.openjdk.java.net/~vlivanov/8003135/simple/webrev.00
>>> 31 lines changed: 16 ins; 6 del; 9 mod
>>>
>>> In the IR produced by Thread.isInterrupted(Z)Z intrinsic it's
>>> possible to hoist the load of _interrupted flag out of the loop. The
>>> fix is to add a barrier to forbid such optimization.
>>>
>>> Proposed fix is cleaner that [1] (no explicit memory state management).
>>> The place for the barrier is deliberately chosen earlier than
>>> necessary and it blocks hoisting of some loop invariants when
>>> Thread.isInterrupted() is called in a loop. But I think it's a fair
>>> price since I don't think that Thread.isInterrupted(Z)Z performance
>>> in tight loops is critical.
>>
>> I don't see any new barrier in the simple fix.  Where is it?  It looks
>> like the load of thr._interrupted can be hoisted under the same
>> circumstances as before.  If the slow path is somehow turned into a
>> loop exit (via uncommon trap, maybe), then the memory state will fold
>> up and the bit load can be hoisted.
>>
>> It seems like your "fast" fix is more robust, for this reason.
>>
>> Also, it's not your bug, but I'm not convinced that "slow_val =
>> intcon(1)" is correct.  I think another thread could come in and clear
>> the interrupted bit, and this call to the native function could return
>> false.
>>
>>> If you prefer [1] let me know.
>>>
>>> Also, cleaned up the code around a little.
>>
>> Good cleanups.
>>
>> — John
>>
>>> Testing: failing test, manual (verified generated code), JPRT (in
>>> progress)
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] http://cr.openjdk.java.net/~vlivanov/8003135/fast/webrev.00
>>