RFR(S): 8069191: moving predicate out of loops may cause array accesses to bypass null check

John Rose john.r.rose at oracle.com
Thu Apr 16 21:19:10 UTC 2015


On Apr 16, 2015, at 3:56 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
> Thanks for the review, comments and suggestions. I followed all suggestions except the one to use assert_dom because I’m not sure how applicable it is here. I added:
> 
> assert(is_dominator(bn, bm) || is_dominator(bm, bn), "one must dominate the other”);
> 
> instead. This is what I intend to push unless I hear an objection:
> 
> http://cr.openjdk.java.net/~roland/8069191/webrev.02/ <http://cr.openjdk.java.net/~roland/8069191/webrev.02/>
I'm happy to see this change go in, because it simplifies the code base significantly.

The code structure and assertions in gcm.cpp and compile.cpp are cleaner; thanks.

(I keep wanting to suggest a little ad hoc de-deduplication of control edges.  E.g., if c==n->in(0), don't do n->add_prec(c).  Is that overly picky?  If done awkwardly it could make the code much harder to read.  Could have a subroutine n->ensure_control_or_add_prec(c).)

As we discussed on Skype, "case Op_CheckCastPP" should be accompanied by "case Op_CastPP", since CastPP nodes can cascade (non-trivially).

I think I understand the whole patch except for this line:
+     if (n->in(0) != NULL && (n->in(0)->is_Region() || (n->in(0)->in(0) != NULL && n->in(0)->in(0)->is_If()))) {

I would prefer a named predicate:

+     if (must_retain_control(n->in(0))) {

Then, the local routine can have the necessary comments explaining why some controls can be neglected while others must be retained.

And, along those lines, I'm having trouble imagining examples of controls that are *not* retained, let along understand why they are less important than Regions (OK, those are always important), and If-projections.  Why not (for example) catch projections?

Thanks!
— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150416/37873f30/attachment.html>


More information about the hotspot-compiler-dev mailing list