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

[John Rose]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150413/2f45a058/attachment.html>