RFR(S): 8134031: Incorrect JIT compilation of complex code with inlining and escape analysis
Roland Westrelin
roland.westrelin at oracle.com
Wed Sep 2 08:56:16 UTC 2015
Thanks for looking at this, Vladimir.
> The fix seems correct. I think it req() was used assuming that new slices will be added only in following code. And then Note2: was added when I realized that previous code may add them but req() was not changed.
>
> What is node 129? MergeMem and Phi 233 point to it.
It’s:
129 Proj === 126 [[ 276 235 142 143 233 234 275 274 175 188 ]] #2 Memory: @BotPTR *+bot, idx=Bot; !orig=[166],[273],[196] !jvms: TestEABadMergeMem::test @ bci:26
126 CallStaticJava === 116 43 57 8 1 ( 1 123 1 1 1 15 16 49 1 1 ) [[ 127 128 129 ]] # Static TestEABadMergeMem::m_notinlined void ( ) TestEABadMergeMem::test @ bci:26 !jvms: TestEABadMergeMem::test @ bci:26
The first m_notinlined() call
> How node 129 references StoreI 110 and StoreI 125? StoreI 110 has 2 users, may be there is bypass.
The call’s memory is:
57 MergeMem === _ 1 7 47 125 [[ 126 ]] { N47:rawptr:BotPTR N125:TestEABadMergeMem$Box+12 * } Memory: @BotPTR *+bot, idx=Bot; !jvms: TestEABadMergeMem::test @ bci:4
So 129 gets to 110 through 125.
The other use for 110 is:
113 MergeMem === _ 1 7 47 110 [[ 118 ]] { N47:rawptr:BotPTR N110:TestEABadMergeMem$Box+12 * } Memory: @BotPTR *+bot, idx=Bot; !jvms: TestEABadMergeMem::test @ bci:23
which is used by an uncommon trap:
118 CallStaticJava === 117 43 113 8 9 ( 99 1 1 1 1 1 1 1 1 1 1 91 14 ) [[ 119 ]] # Static uncommon_trap(reason='null_check' action='maybe_recompile') void ( int ) C=0.000100 TestEABadMergeMem::test @ bci:23 reexecute !jvms: TestEABadMergeMem::test @ bci:23
for the null check of c.i = k
Roland.
>
> Thanks,
> Vladimir
>
> On 8/31/15 8:33 AM, Roland Westrelin wrote:
>> That bug is caused by a bad rewiring of memory edges during ConnectionGraph::split_unique_types()
>>
>> http://cr.openjdk.java.net/~roland/8134031/webrev.00/
>>
>> See test case. Before EA, the “test” method has 3 stores to 3 different Box Objects:
>>
>> 108 StoreI === 97 47 107 12 [[ 110 ]] @TestEABadMergeMem$Box+12 *, name=i, idx=4; Memory: @TestEABadMergeMem$Box:NotNull+12 *, name=i, idx=4; !jvms: TestEABadMergeMem::test @ bci:11
>> 110 StoreI === 97 108 109 13 [[ 125 113 ]] @TestEABadMergeMem$Box+12 *, name=i, idx=4; Memory: @TestEABadMergeMem$Box:NotNull:exact+12 *, name=i, idx=4; !jvms: TestEABadMergeMem::test @ bci:17
>> 125 StoreI === 116 110 124 14 [[ 57 ]] @TestEABadMergeMem$Box+12 *, name=i, idx=4; Memory: @TestEABadMergeMem$Box:NotNull+12 *, name=i, idx=4; !jvms: TestEABadMergeMem::test @ bci:23
>>
>> chained through their memory inputs.
>>
>> It also has 2 loads from 2 different Box objects:
>>
>> 248 LoadI === _ 233 109 [[ 249 ]] @TestEABadMergeMem$Box+12 *, name=i, idx=4; #int !jvms: TestEABadMergeMem::test @ bci:87
>> 246 LoadI === _ 233 124 [[ 249 ]] @TestEABadMergeMem$Box+12 *, name=i, idx=4; #int !jvms: TestEABadMergeMem::test @ bci:82
>>
>> The memory input of both loads is a Phi:
>>
>> 233 Phi === 153 274 129 [[ 150 21 246 248 ]] #memory Memory: @BotPTR *+bot, idx=Bot; !jvms: TestEABadMergeMem::test @ bci:76
>>
>> Input 1 of the Phi is:
>>
>> 274 MergeMem === _ 1 129 1 1 1 1 275 276 [[ 233 ]] { - - - - N275:java/lang/Class:exact+104 * N276:java/lang/Class:exact+108 * } Memory: @BotPTR *+bot, idx=Bot; !orig=[196] !jvms: TestEABadMergeMem::test @ bci:47
>>
>> The Box that is allocated in the “test” method doesn’t escape. ConnectionGraph::split_unique_types() assigns a unique type to that allocation and rewires the memory edges.
>>
>> In:
>>
>> // Phase 2: Process MemNode's from memnode_worklist. compute new address type and
>> // compute new values for Memory inputs (the Memory inputs are not
>> // actually updated until phase 4.)
>>
>> LoadI 248 is processed with:
>>
>> Node *mem = find_inst_mem(n->in(MemNode::Memory), alias_idx, orig_phis);
>>
>> which goes through the Phi 233 to MergeMem 274 and in ConnectionGraph::find_inst_mem(), an edge is added to the MergeMem node:
>>
>> result = find_inst_mem(result, alias_idx, orig_phis);
>> mmem->set_memory_at(alias_idx, result);
>>
>> 274 MergeMem === _ 1 129 1 1 1 1 275 276 110 [[ 233 ]] { - - - - N275:java/lang/Class:exact+104 * N276:java/lang/Class:exact+108 * N110:TestEABadMergeMem$Box:NotNull:exact+12 *,iid=32 } Memory: @BotPTR *+bot, idx=Bot; !orig=[196] !jvms: TestEABadMer\
>> geMem::test @ bci:47
>>
>> for the new unique type.
>>
>> In:
>>
>> // Phase 3: Process MergeMem nodes from mergemem_worklist.
>> // Walk each memory slice moving the first node encountered of each
>> // instance type to the the input corresponding to its alias index.
>>
>> MergeMem 274 is processed. We go over all of its inputs, including the newly added one above. When that input is processed, the:
>>
>> while (mem->is_Mem()) {
>>
>> loop iterates until store 108 (110 is for the new unique type) and the MergeMem is updated again:
>>
>> MergeMem === _ 1 129 1 108 1 1 275 276 110 [[ 233 ]] { - N108:TestEABadMergeMem$Box+12 * - - N275:java/lang/Class:exact+104 * N276:java/lang/Class:exact+108 * N110:TestEABadMergeMem$Box:NotNull:exact+12 *,iid=32 } Memory: @BotPTR *+bot, idx=Bot;\
>> !orig=[196] !jvms: TestEABadMergeMem::test @ bci:47
>>
>> But now the MergeMem is incorrect: LoadI 246 now takes its memory state from that MergeMem through Phi 233 and for LoadI 246, the memory state is StoreI 108 which is before StoreI 125 that sets the field.
>>
>> The compiler then uses split through Phi to optimize the if (flag3) { test in the test case which causes LoadI 246 to move through Phi 233 and a bad value to be loaded.
>>
>> I think the root cause is that we process the input added to the MergeMem in Phase 2 in Phase 3. The fix I propose prevents that.
>>
>> Roland.
>>
More information about the hotspot-compiler-dev
mailing list