How to reliably pin a node in CFG?

[Vladimir Kozlov]

On 11/16/12 14:25, Vladimir Ivanov wrote:
> Vladimir K.,
>
> Thanks, making depends_only_on_test == true for the load solves hoisting
> problem.
>
> Original problem still persist (the check is hoisted [1]), but that's
> another story - invariance analysis in
> PhaseIdealLoop::loop_predication_impl doesn't respect pinning we do.

loop_predication moves checks before the loop. In you case it is after. 
So it is something else.

>
> However, I wasn't able to figure out how to work with
> insert_mem_bar(Op_MemBarCPUOrder). How the graph should look like?

membar has 2 projections (ProjNode): control and memory. Load should 
points to both of them. In your graph only control edge of Load is 
membar projection.

Vladimir

>
> It differs from volatile fields which produce multiple MemBar* nodes
> scattered around in the graph and other MemBarCPUOrderNode usages in
> library_call.cpp didn't give me any hint.
>
> If I issue it before the load [2], compilation fails during late
> scheduling [3]. I tried to issue it after the load in different places,
> but it didn't work (different assertions/compilation aborts during late
> scheduling).
>
> Best regards,
> Vladimir Ivanov
>
> [1]
> http://cr.openjdk.java.net/~vlivanov/8003135/graphs/loadnode_depends_on_test_true_after.png
>
>
> Generated code:
> 056 B6: # B6 <- B5 B6 top-of-loop Freq: 1e-35
> 056 movl R8, [R10 + #28 (8-bit)] # int
> 05a testl rax, [rip + #offset_to_poll_page] # Safepoint: poll for GC #
> InterruptedVisibilityTest::think @ bci:4 L[0]=RBP STK[0]=R8
> # OopMap{rbp=Oop off=90}
> 060 jmp,s B6
>
> [2]
> http://cr.openjdk.java.net/~vlivanov/8003135/graphs/isInterrupted_membar_before.png
>
>
> [3] 1696 3 b java.lang.Thread::isInterrupted (6 bytes) COMPILE SKIPPED:
> late schedule failed: incorrect graph (not retryable)
>
> On 11/15/12 11:24 PM, Vladimir Kozlov wrote:
>> PhaseIdealLoop::dominated_by() and IfNode::dominated_by() are correct in
>> general case. The problem is value of depends_only_on_test(). This load
>> is from RAW memory and we should not hoist it. See comment in LoadPNode
>> class declaration. So we can widen the case to all loads - we have to
>> confirm this with John.
>>
>> An other simple way to fix it is to add membar before load since it is
>> volatile field:
>>
>> insert_mem_bar(Op_MemBarCPUOrder);
>>
>> Vladimir K.
>>
>> On 11/15/12 07:57, Vladimir Ivanov wrote:
>>> Hi,
>>>
>>> I'm looking at 8003135 [1] and the problem is that during null check
>>> (#132) hoisting, the load (#172) is also hoisted [2] [3]. LoadI node is
>>> constructed in LibraryCallKit::inline_native_isInterrupted
>>> and is pinned to IfTrue projection (#133) of #132, but it doesn't help.
>>>
>>> I'm not sure how to avoid the hoisting of the load.
>>>
>>> Should the load be pinned to corresponding Region/Loop node instead of
>>> If node?
>>>
>>> Is the logic in PhaseIdealLoop::dominated_by which updates
>>> control-dependent nodes (:242-:255)is correct?
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] https://jbs.oracle.com/bugs/browse/JDK-8003135
>>> "HotSpot inlines and hoists the Thread.currentThread().isInterrupted()
>>> out of the loop"
>>>
>>> [2]
>>> http://cr.openjdk.java.net/~vlivanov/8003135/graphs/loop_invariant_hoisting_132_before.png
>>>
>>>
>>>
>>>
>>> [3]
>>> http://cr.openjdk.java.net/~vlivanov/8003135/graphs/loop_invariant_hoisting_132_after.png
>>>
>>>
>>>