How to reliably pin a node in CFG?

[Vladimir Ivanov]

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.

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

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