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

[Vladimir Kozlov]

On 12/13/12 4:55 PM, Vladimir Ivanov wrote:
> 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))?

Never mind this. We can't remove slow_region because there are 2 input 
paths.

>
>  > If you rearrange code to generate slow call first and the load on
> fast path last you may not need to preserve init_mem.

It looks like "fast" approach is much more complicated than I thought. 
OK, go with "simple" case but move insert_mem_bar down (below enum) 
where we can see it.

Your comments for enum values are incorrect. !TLS... should be switched 
like next:

      no_int_result_path   = 1, // t == Thread.current() && 
!TLS._osthread._interrupted
      no_clear_result_path = 2, // t == Thread.current() && 
TLS._osthread._interrupted && !clear_int

Thanks,
Vladimir K.

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