RFR(M): 8146612: C2: Precedence edges specification violated

[Doerr, Martin]

Hi Vladimir,

thanks for reviewing and sponsoring.

Best regards,
 Martin

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Freitag, 8. Januar 2016 20:47
To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR(M): 8146612: C2: Precedence edges specification violated

Very good. I will sponsor it.

Thanks,
Vladimir

On 1/8/16 3:06 AM, Doerr, Martin wrote:
> Hi Vladimir,
>
> thanks for the review.
>
> I have changed the comments, added assertions and factored out the common functionality of del_req(), del_req_ordered() and rm_prec() into a new private function close_prec_gap_at(). That makes sense.
>
> About your concern about accessing outside of _in array in rm_prec():
> Please note that i is decremented before it gets used:
> "j == _max-1", "i" will be set to "_max", but decremented in "_in[--i]"
>
> Anyway, I have replaced this code by close_prec_gap_at(), so it doesn't matter anymore.
>
> The new webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8146612_C2_prec_edges/webrev.01/
>
> Best regards,
>    Martin
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Donnerstag, 7. Januar 2016 23:08
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR(M): 8146612: C2: Precedence edges specification violated
>
> // Avoid spec violation: multiple prec edge.
>
> I think should be:
>
> // Avoid spec violation: duplicated prec edge.
>
> Should we add assert to rm_prec()?:
>        assert(j >= _cnt, "not a precedence edge");
>
> Also we may need to check that input index is < _max in set_prec() and rm_prec().
>
> Next access will be outside _in array if j == _max-1 (in rm_prec()):
>
>      _in[i] = NULL;                // NULL out last element
>
> unless we guarantee that there is always NULL at the end. Which I don't see because set_prec() may set the last prec
> edge to not NULL.
>
> Please factor out similar code (search for last non-NULL prec edge) in del_req(), del_req_ordered() and rm_prec() into
> separate method.
>
> Thanks,
> Vladimir
>
>
> On 1/7/16 5:45 AM, Doerr, Martin wrote:
>> Hi,
>>
>> some time ago, we found out, that C2 doesn't treat precedence edges as specified.
>>
>> The description of precedence edges in node.hpp says:
>>
>> "They are unordered and not duplicated; they have no embedded NULLs."
>>
>> Some functions in the current implementation violate this specification.
>>
>> I have fixed this in the following webrev:
>>
>> http://cr.openjdk.java.net/~mdoerr/8146612_C2_prec_edges/webrev.00/
>>
>> Please review. I will need a sponsor, please.
>>
>> Best regards,
>>
>> Martin
>>