[9] RFR(M): 8040213: C2 does not put all modified nodes on IGVN worklist

[John Rose]

On Jul 15, 2014, at 7:18 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:

> Can I get another review?
> 
> Thanks,
> Tobias
> 
> On 11.07.2014 08:28, Tobias Hartmann wrote:
>> Hi Vladimir,
>> 
>> thanks for the review.
>> 
>>> Thank you for doing this cleanup, Tobias.
>>> 
>>> Looks good. Small note, in divnode.cpp please use explicit NULL checks:
>>> 
>>> if (in(0) != NULL) {
>> 
>> Done. New webrev:
>> http://cr.openjdk.java.net/~thartmann/8040213/webrev.01/

Loading C->_modified_nodes with a stack address is bad news, even if the code apparently cleans it up on exit.  (I say "apparently" because there are early exits from that block, and the cleanup is not RAII style.)  I strongly suggest heap allocation instead of stack allocation; it's expensive but it's for debug code.

As a matter of style, I would prefer the comment '// Iterative Global Value Numbering, including ideal transforms' and the following declaration to immediately follow the open brace, with the new inserted code placed after the declaration of 'igvn' and before the block which calls 'Optimize'.

I like the simplification of most of the modification sites by factoring patterns into 'replace_input_of'; that is a good cleanup.

The places where 'can_reshape' gates 'rehash_node_delayed' go in the opposite, not-so-good direction; they feel kludgey and hard to understand.  Surely they can be factored too, so that the fiddly logic is centralized.

If you were to make a well-named brother to 'replace_input_of' and make it virtual on PhaseTransform or PhaseGVN, wouldn't that put the 'rehash_node_delayed' logic into a more central position?  The flag 'can_reshape' is simply (IIRC) a cached type indication of whether the phase is an IterGVN.

Suggest reducing the number of one-line DEBUG_ONLY() macro calls by using PRODUCT_RETURN:
+  void         record_modified_node(Node* n) PRODUCT_RETURN;
+  void         remove_modified_node(Node* n) PRODUCT_RETURN;
+  Unique_Node_List*   modified_nodes() const { return DEBUG_ONLY(_modified_nodes) NOT_DEBUG(NULL); }

I like that the new asserts apparently found a number of small bugs which you fixed.

You inserted '#ifdef ASSERT' after 'PhaseIterGVN::verify_PhaseIterGVN', but I don't think you need it, since all of the code (including the similar insertion a few lines up) is '#ifndef PRODUCT'.  See also suggestion above regarding 'modified_nodes'.

There are a few places where tests like 'while (modified_list->size())' appears but there is no null check of 'modified_list' itself.  This could cause a crash if we ever get the phasing wrong; consider an assert or explicit null check 'while (modified_list != NULL && ...)'.

— John