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

[Niclas Adlertz]

Thanks Vladimir and Christian.

Kind Regards,
Niclas Adlertz


On 17 okt 2013, at 19:35, Christian Thalinger <christian.thalinger at oracle.com> wrote:

> Looks good.
> 
> On Oct 17, 2013, at 3:11 AM, Niclas Adlertz <niclas.adlertz at oracle.com> 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.
>> 
>