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

[Vladimir Kozlov]

Still good for me.

Thanks,
Vladimir

On 4/16/15 3:56 AM, Roland Westrelin wrote:
> 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
>