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

[Doerr, Martin]

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
>