RFR(XS) 8022783: Nashorn test fails with: assert(!def_outside->member(r))

[Vladimir Kozlov]

Good but split long comment line into two (no need to resend webrev).

Thanks,
Vladimir

On 10/17/13 3:11 AM, Niclas Adlertz wrote:
> Thanks Vladimir.
>
> http://cr.openjdk.java.net/~adlertz/JDK-8022783/webrev01/
>
> Kind Regards,
> Niclas Adlertz
>
> On 2013-10-16 18:22, Vladimir Kozlov wrote:
>> Niclas,
>>
>> You need to change/add comments in this code. Otherwise it is good.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/16/13 7:32 AM, Niclas Adlertz wrote:
>>> Hi all,
>>>
>>> Despite the fact that I have not been able to reproduce JDK-8022783, it generates the same error as many other bugs;
>>> JDK-7033056, JDK-8016269, JDK-8011770, JDK-8005956, JDK-8016157 and JDK-7196566.
>>> They are all related to re-materialization. One major concern when re-materializing is that each input node to the
>>> re-materialized node might need to have its live range 'L' stretched.
>>> If 'L' is defined at multiple places (due to coalescing), we might stretch one definition of 'L' beyond another
>>> definition.
>>>
>>> Currently we handle this by doing a private copy of the input in the same block as the node that is being
>>> re-materialized. This happens at the beginning of PhaseChaitin::Split_Rematerialize:
>>>
>>>    // The input live ranges will be stretched to the site of the new
>>>    // instruction.  They might be stretched past a def and will thus
>>>    // have the old and new values of the same live range alive at the
>>>    // same time - a definite no-no.  Split out private copies of
>>>    // the inputs.
>>>    if( def->req() > 1 ) {
>>>      for( uint i = 1; i < def->req(); i++ ) {
>>>        Node *in = def->in(i);
>>>        // Check for single-def (LRG cannot redefined)
>>>        uint lidx = _lrg_map.live_range_id(in);
>>>        if (lidx >= _lrg_map.max_lrg_id()) {
>>>          continue; // Value is a recent spill-copy
>>>        }
>>>        if (lrgs(lidx).is_singledef()) {
>>>          continue;
>>>        }
>>>
>>>        Block *b_def = _cfg.get_block_for_node(def);
>>>        int idx_def = b_def->find_node(def);
>>>        Node *in_spill = get_spillcopy_wide( in, def, i );
>>>        if( !in_spill ) return 0; // Bailed out
>>>        insert_proj(b_def,idx_def,in_spill,maxlrg++);
>>>        if( b_def == b )
>>>          insidx++;
>>>        def->set_req(i,in_spill);
>>>      }
>>>    }
>>>
>>> We do not need to do this for live ranges with only a single definition.
>>> We also do not do this for recently created spill copies. Recent spill copies refers to spill copies created in the
>>> current split pass, that have not yet been assigned live ranges.
>>>
>>> The problem is that spill copies may introduce phi nodes due to different reaching definitions from different control
>>> paths. And phi nodes introduces coalescing; in the end of a split pass we coalesce phi inputs with the phi itself (for
>>> all phi nodes created in the split pass).
>>> So what happens if a phi input is a recent spill copy? The recent spill copy get coalesced with the phi (and the live
>>> range of the the spill copy and the phi will be multi defined).
>>> And what happens if this recent spill copy is input to a re-materialized node? Then we introduce the risk of stretching
>>> the live range beyond another def. (*)
>>>
>>> This is exactly what happens in prior bugs JDK-8005956 and JDK-8016157. When I solved them, I did not know well enough
>>> what the problem was, so I disabled re-materialization for some nodes. Now I know better and instead I can add the same
>>> support for recent spill copies, i.e. I create private copies for them as well. At the same time, I can remove the old
>>> temporary solutions.
>>>
>>> I have verified that JDK-8005956 now also works with the new solution. Unfortunately I have not been able to reproduce
>>> JDK-8016157, but I have been able to solve a new bug, JDK-7196566, using this new solution.
>>>
>>> I have also verified that this fix does not give any performance regressions.
>>> http://sthaurora-ds.se.oracle.com/performance/rest/submit/runStatus?name=REG_SPLIT
>>> http://sthaurora-ds.se.oracle.com/performance/rest/perf/report?name=REG_SPLIT
>>>
>>> WEBREV: http://cr.openjdk.java.net/~adlertz/JDK-8022783/webrev00/
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8022783
>>>
>>> Kind Regards,
>>> Niclas Adlertz
>>>
>>> (*) It is tricky because this can not happen by just coalescing a recent spill copy (as phi input) with its phi node
>>> 'P'. It happens because the input for 'P' is also a phi, which is THEN coalesced together with the recent spill copy.
>>> Simply put, we need more than one recursion of coalescing in the split pass for this to happen.
>