RFR(S): 8087128 C2: Disallow definition split on MachCopySpill nodes

[Nils Eliasson]

Thank you Vladimir,

// Nils

On 2019-01-29 23:48, Vladimir Kozlov wrote:
> Thank you, Nils
>
> Looks good to me.
>
> Please run hs-precheckin-comp testing too.
>
> Vladimir
>
> On 1/28/19 7:15 AM, Nils Eliasson wrote:
>> Hi,
>>
>> We have a problem that we sometimes hit an assert in reg_split.cpp.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8087128
>>
>> http://cr.openjdk.java.net/~neliasso/8087128/webrev.01/
>>
>> We have a block that looks like this:
>>
>> 1262: #        B1264 B1263 <- N7283  Freq: 0,0927179
>>   7282   Region  ===  7282  1704  [[ 7282  1702  1715 ]]
>>   11500  MemToRegSpillCopy       === _  11190  [[ 9184 ]] 
>> Oop:com/sun/tools/javac/$
>>   9184   DefinitionSpillCopy     === _  11500  [[ 9185  1702 11503 
>> 11505 ]]   Oop:$
>>   11501  MemToRegSpillCopy       === _  9169  [[ 11665  11506 11504 
>> 11502 ]]   Oop$
>>   11645  MemToRegSpillCopy       === _  9167  [[ 9182 ]] 
>> Oop:com/sun/tools/javac/c$
>>   9182   DefinitionSpillCopy     === _  11645  [[ 1702  11646 11647 
>> 11648 ]]   Oop$
>>   1715   checkCastPP     ===  7282  1716  [[ 1702 ]] 
>> java/util/HashMap$TreeNode:NotN$
>>   9185   BoundSpillCopy  === _  9184  [[ 1702 ]] 
>> Oop:com/sun/tools/javac/code/Symb$
>>   11665  RegToMemSpillCopy       === _  11501  [[ 1702 ]] 
>> Oop:com/sun/tools/javac/$
>>   1702   CallStaticJavaDirect    ===  7282  185  182  16  0 1715 187 
>> 9185  11665 $
>>   1703   MachProj        ===  1702  [[]] #10006/fat
>>
>>
>> 11501 "MemToRegSpillCopy" has one use in this block, "11665 
>> RegToMemSpillCopy", and three uses in other blocks. The use "11665 
>> RegToMemSpillCopy" is used by "1702 CallStaticJavaDirect".
>>
>> We hit the assert when processing 11501 in PhaseChaitin::Split.
>>
>> We are in the "Handle DEFS" section of the split routine. We only get 
>> here if the live range has been marked as spilled when the coloring 
>> have ran out of colors. There are two code paths, one default, where 
>> we just record the def and updates the side tables, and one where we 
>> do a definition split on the live range. This split is guarded by 
>> several conditions. In this case we get here by having a register 
>> mask that is only regs (UP) and by being in a high pressure region. 
>> Everything seems ok, so why don't we end up with MachSpillCopies here 
>> more often?
>>
>> Also - one of the 4 uses is in this block (11655) and it's a 
>> reg-to-mem already. It doesn't make much sense to add even more 
>> spills here. Why does this happen?
>>
>> UseFPUForSpilling added a restriction to coalesing - it skips 
>> coalescing when the two live ranges have different pressure. The 
>> reason for this is that with FPU spilling, the possible extra 
>> spilling is for "free". (I can't find any documentation on benchmarks 
>> where this is beneficial though.) The downside is that we get longer 
>> spill chains like: DefSpill-memToReg-RegToMem-MemToReg, that doesn't 
>> collapse, because of pressure changes. This may cause the live ranges 
>> defined by memToReg-nodes to become spilled, and if we are in a 
>> high-pressure region - we hit the assert.
>>
>> So my conclusion is that nothing is really wrong. Everything still 
>> works without the assert. The spill-chains are unnecessary long, but 
>> only because we have chosen to restrict the coalescing. But we 
>> shouldn't split the spill-nodes even more. In the next iteration the 
>> coalescing within the block will have reduced the chains, and later a 
>> proper coloring will be found.
>>
>> My solution is that we prevent the MachSpillCopies (only Mem-To-Regs 
>> can end up here) from being split again. This is ok - because this is 
>> exactly what would have happened if we would have been in a low 
>> pressure region.
>>
>> I have done some measurements and it doesn't increase the number of 
>> spill-iterations.
>>
>> Regards,
>>
>> Nils
>>
>>
>>