How to reliably pin a node in CFG?
Vladimir Kozlov
Vladimir.Kozlov at oracle.com
Tue Nov 20 12:26:47 PST 2012
The problem is that we need to reset memory for the call otherwise it
use memory proj of membar on an OTHER code branch:
40 MemBarCPUOrder === 34 1 7 1 1 [[ 41 42 ]] !jvms:
Thread::isInterrupted @ bci:2
41 Proj === 40 [[ 47 44 ]] #0 !jvms: Thread::isInterrupted @
bci:2
42 Proj === 40 [[ 70 44 56 70 ]] #2 Memory: @BotPTR
*+bot, idx=Bot; !jvms: Thread::isInterrupted @ bci:2
56 CallStaticJava === 33 6 42 8 1 ( 10 22 1 ) [[ 57 58
59 61 ]] # Static java.lang.Thread::isInterrupted bool (
java/lang/Thread:NotNull *, bool ) Thread::isInterrupted @ bci:2 !jvms:
Thread::isInterrupted @ bci:2
Something like following patch but I don't like it. Look on other
similar code and find better solution.
Thanks,
Vladimir
src/share/vm/opto/library_call.cpp Tue Nov 20 12:22:58 2012 -0800
@@ -3147,7 +3147,12 @@
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()));
+
// Set the control input on the field _interrupted read to prevent
it floating up.
+ Node* init_mem = map()->memory();
+ insert_mem_bar(Op_MemBarCPUOrder);
+ Node* fast_mem = map()->memory();
+
Node* int_bit = make_load(control(), p, TypeInt::BOOL, T_INT);
Node* cmp_bit = _gvn.transform( new (C) CmpINode(int_bit, intcon(0)) );
Node* bol_bit = _gvn.transform( new (C) BoolNode(cmp_bit,
BoolTest::ne) );
@@ -3186,6 +3191,7 @@
result_val->init_req(slow_result_path, top());
} else {
// non-virtual because it is a private non-static
+ set_all_memory(init_mem);
CallJavaNode* slow_call =
generate_method_call(vmIntrinsics::_isInterrupted);
Node* slow_val = set_results_for_java_call(slow_call);
@@ -3195,7 +3201,6 @@
if (known_current_thread) slow_val = intcon(1);
Node* fast_io = slow_call->in(TypeFunc::I_O);
- Node* fast_mem = slow_call->in(TypeFunc::Memory);
// These two phis are pre-filled with copies of of the fast IO and
Memory
Node* io_phi = PhiNode::make(result_rgn, fast_io, Type::ABIO);
Node* mem_phi = PhiNode::make(result_rgn, fast_mem, Type::MEMORY,
TypePtr::BOTTOM);
On 11/20/12 05:45, Vladimir Ivanov wrote:
> >> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
More information about the hotspot-compiler-dev
mailing list