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

[Vladimir Kozlov]

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