Request for Reviews(M): 7092905: C2: Keep track of the number of dead nodes

[Vladimir Ivanov]

Few minor comments:

src/share/vm/opto/compile.cpp:
+static inline bool NotANode(const Node* n) {
Can you rename it to something like not_a_node? Camel style names aren't 
common in the codebase.

    if (_log != NULL) {
-    _log->done("phase nodes='%d'", C->unique());
+    _log->done("phase name='%s'", _phase_name);
    }
Phase name is redundant here - it duplicates info from the head. But I 
would leave nodes count as is and add live nodes count.

Best regards,
Vladimir Ivanov

On 11/20/12 10:39 PM, Bharadwaj Yadavalli wrote:
>
> Please review the updated webrev at
> http://cr.openjdk.java.net/~bharadwaj/7092905/webrev_03/
>
> Hopefully I have addressed all the review comments/suggestions.
>
> Thanks,
>
> Bharadwaj
>
> On 11/9/2012 12:35 PM, Vladimir Kozlov wrote:
>> Bharadwaj,
>>
>> Nils just reminded me (for his changes) that you need to fix our
>> LogCompilation parsing tool to recognize new "unique='n'
>> live_tracked='n'" values (old was "nodes='n'"):
>>
>> src/share/tools/LogCompilation/
>>
>> Also live_tracked could be simple "live".
>>
>> Thanks,
>> Vladimir
>>
>> Vladimir Kozlov wrote:
>>> VerifyIdealNodeCount flag should be also notproduct since it is used
>>> only in debug VM.
>>>
>>> The next assert is not needed because it duplicates assert in
>>> live_nodes():
>>>
>>>     bool check_node_count(uint margin, const char* reason) {
>>>       if (unique() + margin > (uint)MaxNodeLimit) {
>>> +     assert(dead_node_count() <= unique(), "number of dead nodes
>>> more than created");
>>>
>>> Also don't split the assert in live_nodes() into 2 lines and maybe
>>> use err_msg_res() to print _unique and _dead_node_count values. Also
>>> I think it should return unsigned int type value as unique() method.
>>>
>>> Second Compile() constructor does not set _dead_node_count(0).
>>>
>>> count_live_nodes_by_graph_walk() and NotANode() methods are now used
>>> only in debug code so put them under #ifdef ASSERT.
>>>
>>>
>>> print_missing_nodes() should call count_live_nodes_by_graph_walk()
>>> instead of duplicating code. And it should print to _log itself.
>>> TracePhase() code will have only call to it:
>>>
>>> 3133   if (VerifyIdealNodeCount) {
>>> 3134     Compile::current()->print_missing_nodes();
>>> 3135   }
>>>
>>> And do not check VerifyIdealNodeCount in print_missing_nodes() to
>>> allow call it in debugger without VerifyIdealNodeCount set.
>>>
>>> I think VerifyIdealNodeCount code should be in ~TracePhase() (end of
>>> phase).
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> Bharadwaj Yadavalli wrote:
>>>> Addressed comments on my previous RFR. Changes at
>>>> http://cr.openjdk.java.net/~bharadwaj/7092905/webrev_02/
>>>>
>>>> These changes are made to keep an (almost) accurate running count of
>>>> the reachable (live) flow graph nodes. This will result in a more
>>>> realistic node count for various phases of C2 to decide on whether
>>>> to proceed with optimizations or not. Prior to these changes, C2
>>>> bails out of compilation based on the number of nodes created which
>>>> typically larger than the number of reachable (live) nodes.
>>>>