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

[Vladimir Kozlov]

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