[16] RFR(S): 8249603: C1: assert(has_error == false) failed: register allocation invalid

[Christian Hagedorn]

Hi Tobias

Thanks a lot!

I think we will always catch an overlap later in the verification method 
unless we somehow correct the mistake until then. But I don't think that 
this is likely or even possible. Nevertheless, I still wanted to verify 
that to some extent and added an assert(false) in the newly added 
intersection bailout test with the split children and could not trigger 
it in tier 1-4 (apart from the newly added test).

Best regards,
Christian

On 10.08.20 10:13, Tobias Hartmann wrote:
> Hi Christian,
> 
> I agree with Vladimir, very nice analysis. Although I'm not too familiar with the C1 register
> allocator, your explanation and fix makes sense to me.
> 
> Just wondering, do we hit this case with any of our existing tests?
> 
> Best regards,
> Tobias
> 
> On 06.08.20 11:34, Christian Hagedorn wrote:
>> Hi
>>
>> Please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8249603
>> http://cr.openjdk.java.net/~chagedorn/8249603/webrev.00/
>>
>> Register allocation fails in C1 in the testcase because two intervals overlap (they both have the
>> same stack slot assigned). The problem can be traced back to the optimization to assign the same
>> spill slot to non-intersecting intervals in LinearScanWalker::combine_spilled_intervals().
>>
>> In this method, we look at a split parent interval 'cur' and its register hint interval
>> 'register_hint'. A register hint is present when the interval represents either the source or the
>> target operand of a move operation and the register hint the target or source operand, respectively
>> (the register hint is used to try to assign the same register to the source and target operand such
>> that we can completely remove the move operation).
>>
>> If the register hint is set, then we do some additional checks and make sure that the split parent
>> and the register hint do not intersect. If all checks pass, the split parent 'cur' gets the same
>> spill slot as the register hint [1]. This means that both intervals get the same slot on the stack
>> if they are spilled.
>>
>> The problem now is that we do not consider any split children of the register hint which all share
>> the same spill slot with the register hint (their split parent). In the testcase, the split parent
>> 'cur' does not intersect with the register hint but with one of its split children. As a result,
>> they both get the same spill slot and are later indeed both spilled (i.e. both virtual
>> registers/operands are put to the same stack location at the same time).
>>
>> The fix now additionally checks if the split parent 'cur' does not intersect any split children of
>> the register hint in combine_spilled_intervals(). If there is such an intersection, then we bail out
>> of the optimization.
>>
>> Some standard benchmark testing did not show any regressions.
>>
>> Thank you!
>>
>> Best regards,
>> Christian
>>
>>
>> [1] http://hg.openjdk.java.net/jdk/jdk/file/7a3522ab48b3/src/hotspot/share/c1/c1_LinearScan.cpp#l5728