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>