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

Thu May 17 19:20:38 UTC 2018

Okay 

Thanks 
Vladimir 

> On May 17, 2018, at 2:52 AM, Nils Eliasson <nils.eliasson at oracle.com> wrote:
> 
> 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
>>>>>>>> 
>