How to reliably pin a node in CFG?

[Vladimir Ivanov]

 >> 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.
Actually, it does move the check before the loop. The reason why 
invariance analysis concludes that the load is invariant is because of 
control node (IfTrue(#133)/If(#132)) the load depends on is marked as 
invariant. It occurs on a previous iteration when IfNode/#132 was 
predicated and became #211 (old nodes are marked as invariant in 
Invariance::map_ctrl called from PhaseIdealLoop::loop_predication_impl).

 >>
 >> 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.
I double-checked and Load actually has both projections [1]. Any other 
evident mismatch in the graph? I'm debugging the code which does late 
scheduling but haven't found the root cause yet.

Just in case: the patch [2] and the test [3] I use.

Best regards,
Vladimir Ivanov

[1] 
http://cr.openjdk.java.net/~vlivanov/8003135/graphs/isInterruped_with_membar.png

[2] diff --git a/src/share/vm/opto/library_call.cpp 
b/src/share/vm/opto/library_call.cpp
--- a/src/share/vm/opto/library_call.cpp
+++ b/src/share/vm/opto/library_call.cpp
@@ -3250,6 +3250,9 @@
    Node* p = basic_plus_adr(top()/*!oop*/, tls_ptr, 
in_bytes(JavaThread::osthread_offset()));
    Node* osthread = make_load(NULL, p, TypeRawPtr::NOTNULL, T_ADDRESS);
    p = basic_plus_adr(top()/*!oop*/, osthread, 
in_bytes(OSThread::interrupted_offset()));
+
+  insert_mem_bar(Op_MemBarCPUOrder);
+
    // Set the control input on the field _interrupted read to prevent 
it floating up.
    Node* int_bit = make_load(control(), p, TypeInt::BOOL, T_INT);
    Node* cmp_bit = _gvn.transform( new (C) CmpINode(int_bit, intcon(0)) );

[3] InterruptedVisibilityTest.java
public class InterruptedVisibilityTest {
     public void think() {
         while (true) {
             if (checkInterruptedStatus()) break;
         }
         System.out.println("We're done thinking");
     }

     private boolean checkInterruptedStatus() {
         return Thread.currentThread().isInterrupted();
     }

     public static void main(String[] args) throws InterruptedException {
         final InterruptedVisibilityTest test = new 
InterruptedVisibilityTest();
         test.think();
     }
}


On 11/17/12 1:58 AM, Vladimir Kozlov wrote:
> 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
>>>>
>>>>
>>>>
>>>>