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

[Nils Eliasson]

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