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

[Tobias Hartmann]

Hi John,

thanks a lot for the detailed review! Please see comments inline.

On 16.07.2014 01:10, John Rose wrote:
> 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.

There are other places in the code where this is done, for example in 
Compile::Compile(..) the Unique_Node_List 'for_igvn' is created on the 
stack. Is this a special case?

I'm not sure how to correctly allocate the object on the heap. Do I have 
to use new/delete or is this handled by some magic in "ResourceObject'?

> 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'.

Okay, done.

> 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.

I added an empty virtual 'igvn_rehash_node_delayed' to PhaseTransform 
and implemented it in PhaseIterGVN to move the logic into a more central 
position. I removed the 'can_reshape' checks.

> 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); }

Thanks for the hint, it looks way better now!

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

Yes, I'm a little bit concerned that additional bugs will show up after 
integration, although I did a lot of testing.

> 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'.

True, I removed the #ifdef.

> 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 && ...)'.

I added explicit null checks.


New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.02/

Thanks,
Tobias

> — John