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

[Roland Westrelin]

Hi John,

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/

Roland.


> On Apr 14, 2015, at 5:31 AM, John Rose <john.r.rose at oracle.com> wrote:
> 
> Reviewed.
> 
> On Mar 24, 2015, at 5:55 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>> 
>>>> 
>>>> test guarantees that the precedence edge is a control node. And I assume it’s always ok to remove the precedence edge and adjust the control when the precedence edge is a control node. Do you think that could break something?
>>> 
>>> Only if control edge came from CastPP. I know it is additional work but can you run something (CTW? jvm98) and look what types of precedence edges GCM can see? Unfortunately I don't remember what we have there.
>>> There are a lot of places where we use add_prec(), mostly add pointers to memory nodes.
>>> If control nodes come only from CastPP then I am fine with your code.
>> 
>> I added debugging code (that I didn’t keep in the webrev below) that added (memory operation, control from CastPP) pairs in a side table during final graph reshaping, updated the pairs during matching and checked that all nodes that gcm sees with a control precedence got it from a CastPP. I ran CTW and other tests with that code and all tests passed. During that testing, I noticed that:
> 
> That's a good testing method.
> 
> Precedence edges are a simple way to add miscellaneous node relations but it is easy to forget they are there.  I guess the gcm.cpp code picks them up completely.  And after the extra edges are added, not much happens that could "forget" (drop) an edge.  (Note that copying a node to make a better one has a risk to "forget" precedence edges.)
> 
> But, if this technique were to be used in any more expansive way, or if you have lingering doubts about using precedence edges here, I would recommend creating an explicit new node type that captures multiple control dependency edges.  As we have a MergeMem node we could have a MergeControl node, whose input edges (after in(0)) would act like the precedence edges you are adding now.
> 
> Two minor comments on code style in compile.cpp:  The new 'switch' is hard to untangle.  Wouldn't it be simpler to put the 'wq.push(use)' call before the 'break', and drop the 'default' case completely?
> 
> Also, I really dislike it when block structure ({...}) cuts across #ifdef structure.  This hack would be slightly better:
>    #ifdef _LP64
>       if (n->in(1)->is_DecodeNarrowPtr() || n->in(2)->is_DecodeNarrowPtr()) 
>     ...
>       } else
>   #endif //_LP64
>   {
>      ...
>   }
> 
> Better yet, you could also just delete the #ifdef LP64 and let the tests go forward.  Or incorporate a manifest constant:
>     const bool is_LP64 = LP64_ONLY(true) NOT_LP64(false);
>     if (is_LP64 && (...)) { ... } else { ... }
> 
> The code in gcm.cpp treats precedence edges asymmetrically.  (The expression is 'n = is_dominator(bn, bm) ? m : n'.)  Do we want to assert that one of them dominates the other, perhaps using 'assert_dom'?
> 
> It's great to see all that mysterious old code go away.
> 
> — John