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

[Nils Eliasson]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180314/f67c7142/attachment.html>