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

[Vladimir Kozlov]

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.