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

[Tobias Hartmann]

Hi Nils,

very nice work! I also like the new debugging points.

Small typo in gcm.cpp:711: "actully" -> "actually", "skill" -> "skip"?

Best regards,
Tobias


On 14.03.2018 11:17, 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
>>>
>