RFR(S): 8192992: Test8007294.java failed: attempted to spill a non-spillable item

[Nils Eliasson]

Hi,

Refreshing this review. I ran into a problem running Pack200Test and had 
to reevaluate my fix.

The original problem was that the check at gcm.cpp:686-718 was to 
conservative. Any Phi found, forces the load up, but only when a phi is 
part of a loop can there be an interfering store. Any other phi, would 
be a control flow merge, and stores in a different control flow can't 
interfere.

In my previous webrev I thought I could get away with removing the 
iteration over the phis inputs, since I already knew what op (mem) I 
came from. I missed the detail that (store->in[j] == mem ) doesn't imply 
that (get_block_for_node(store)->pred(j) == get_block_for_node(mem)).

Only changed in gcm.cpp compared to earlier webrevs:

http://cr.openjdk.java.net/~neliasso/8192992/webrev.06/

I have run tier1-3 + hs-preckin-comp + known reproducing tests: 
Pack200Test, compiler/c2/Test6843752.java, Test8007294.java

Regards,

Nils



On 2018-03-21 17:12, Vladimir Kozlov wrote:
> I understand that if Phi is in loop you want to place above loop.
> Do I understand correctly that for regular Phis we don’t need to traverse their inputs because we will visit those inputs anyway when traversing graph?


>
> Thanks,
> Vladimir
>
>> On Mar 21, 2018, at 8:28 AM, Nils Eliasson <nils.eliasson at oracle.com> wrote:
>>
>> This is the new solution:
>>
>> http://cr.openjdk.java.net/~neliasso/8192992/webrev.05
>>
>> I am removing some code I found redundant.
>>
>> In the original code - when finding a phi we visit the phis predecessors. The problem is that we will always find the node we visited before the phi. And since no extra checks are done - this will be the new LCA. If that node is the same as the original loads memory, the LCA will always be the early block.
>>
>> Firstly I remove the iteration over the phis predecessors. If the phi is in a loop, we must follow edges around, just checking the immediate predecessors isn't enough. There might be a store further away. It works only because we always raise the LCA, we don' have to traverse the preds just to do it anyway.
>>
>> I add a check too see if the phi is belonging to a loop. This is the only case where we can find phis in the memory flow, that has predecessors that affects us.
>>
>> The second change is to just raise the LCA before the Phi's block. That is enough - the load is then placed before the loop.
>>
>> http://cr.openjdk.java.net/~neliasso/8192992/webrev.05
>>
>> I will do extensive testing of this.
>>
>> Regards,
>> Nils
>>
>>
>>
>>
>> On 2018-03-16 16:37, Nils Eliasson wrote:
>>> Hi Vladimir,
>>>
>>> I managed to smash right into JDK-6843752 "missing code for an anti-dependent Phi in GCM".
>>>
>>> Thanks for adding that test :)
>>>
>>> I'll be back next week with a solution.
>>>
>>> Regards,
>>>
>>> // Nils
>>>
>>>
>>> On 2018-03-14 18:42, Vladimir Kozlov wrote:
>>>> Very good! Thank you for doing this analysis.
>>>> Please, run our usual mach5 set of tests.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 3/14/18 3:17 AM, Nils Eliasson wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> After taking a very close look I found that the anti-dependency checking that hoists the testN_mem_reg from the jmpCon is broken, and that the hoisting is unnecessary. So this is not a case where we need anti-depenency checking for loads before matching.
>>>>>
>>>>> Generally the insert_anti_dependences looks good, except the store->is_Phi() clause that is full of holes (overly conservative). I don't think I fully understand how the graph looks when the clause is needed, but it tries to find stores upwards that is otherwise unreachable from the downward memory flow search.
>>>>>
>>>>> I found these three flaws:
>>>>>
>>>>> 1) A Phi in a block that is preceded by a store - even though the store is dominated by the loads LCA it will force the testN up! We don't check where the stores are located.
>>>>>
>>>>> 2) A Phi that consumes the same memory as the load may force it up, even though no stores are involved.
>>>>>
>>>>> 3) A Phi that consumes a mergemem, which in itself has already has been processed and passed as irrelevant, may force the testN up.
>>>>>
>>>>> One could add that any predecessor to the phi would have to be a store/call to affect the load placement.
>>>>>
>>>>> I have also added some additional debugging printouts to the patch.
>>>>>
>>>>> http://cr.openjdk.java.net/~neliasso/8192992/webrev.03/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8192992
>>>>>
>>>>> Regards,
>>>>>
>>>>> // Nils
>>>>>
>>>>>
>>>>>
>>>>> On 2018-03-05 18:02, Vladimir Kozlov wrote:
>>>>>> Hi Nils,
>>>>>>
>>>>>> Yes, it is legal workaround but this way you removed all subsuming loads in code.
>>>>>>
>>>>>> Should we do anti-dependency check for loads during matching when shared nodes are marked?:
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk/hs/file/4e82736053ae/src/hotspot/share/opto/matcher.cpp#l2159
>>>>>>
>>>>>> How expensive would be that?
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> On 3/5/18 7:50 AM, Nils Eliasson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This patch is a workaround for a scheduling problem encountered in some rare circumstances. Instead of hitting the assert we retry the compilation without subsuming loads.
>>>>>>>
>>>>>>> To quote Tobias:
>>>>>>>
>>>>>>> "The crash happens because a testN_mem_reg0 (CmpN(LoadN(mem), NULL)) is scheduled in a different block than its jmpCon user and the register allocator tries to spill the flag register. The problem is that PhaseCFG::schedule_late() detects an anti-dependency for the testN_mem_reg0 on a bottom memory Phi and therefore raises the LCA to the early block (see PhaseCFG::insert_anti_dependences()) which is "far away" from its jmpCon user."
>>>>>>>
>>>>>>> Thanks to Roland who suggested the workaround.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8192992
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~neliasso/8192992/webrev.01/
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Nils
>>>>>>>