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

[Vladimir Ivanov]

John, Vladimir K.,

Here's updated version [1]

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~vlivanov/8003135/simple/webrev.01/

On 12/14/12 4:11 AM, Vladimir Kozlov wrote:
> 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
>>>>