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

[Nils Eliasson]

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