RFR: 8290711: assert(false) failed: infinite loop in PhaseIterGVN::optimize [v2]

[Christian Hagedorn]

On Mon, 22 Aug 2022 10:22:16 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In the test case: the loop is an infinite loop but loop optimizations
>> don't add a NeverBranch node because the loop is reachable from Root
>> (following Root's inputs) through a null check in the loop body. When
>> CCP runs, it finds the null check never fails. PhaseCCP::transform()
>> works through the graph starting from Root and following its inputs.
>> But because the null check never fails, it doesn't follow the path to
>> the loop body and it doesn't update the type of nodes in the loop body
>> (as done in PhaseCCP::transform_once()). That's wrong as the loop body
>> is not dead. As a consequence, for one CastPP node, its bottom type
>> and the type recorded in PhaseCCP's type table differ. When igvn runs
>> next, for some nodes, MemNode::Ideal_common() sees a difference
>> between the address type recorded by igvn and the one reported by
>> bottom_type(). That causes memory nodes to be indefinitely
>> re-enqueued for igvn.
>> 
>> The fix I propose is similar to one Tobias implemented in the valhalla
>> repo (JDK-8265973). With this fix, all loops are always considered
>> reachable by PhaseCCP::transform() (which I think, worst case, is only
>> a waste of compilation time but has no correctness issue). To achieve
>> that safepoints are collected by CCP before PhaseCCP::transform()
>> transform() follows inputs from Root and safepoints.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - comment
>  - fix
>  - test

That looks reasonable!

src/hotspot/share/opto/phaseX.cpp line 1970:

> 1968:   for (uint i = 0; i < _safepoints.size(); ++i) {
> 1969:     Node *nn = _safepoints.at(i);
> 1970:     Node *new_node = _nodes[nn->_idx];

Asterisk position:
Suggestion:

    Node* nn = _safepoints.at(i);
    Node* new_node = _nodes[nn->_idx];

src/hotspot/share/opto/phaseX.cpp line 1973:

> 1971:     assert(new_node == NULL, "");
> 1972:     new_node = transform_once(nn);
> 1973:     _nodes.map( nn->_idx, new_node );

Spacing:
Suggestion:

    _nodes.map(nn->_idx, new_node);

src/hotspot/share/opto/phaseX.hpp line 585:

> 583:   static void push_load_barrier(Unique_Node_List& worklist, const BarrierSetC2* barrier_set, const Node* use);
> 584:   void push_and(Unique_Node_List& worklist, const Node* parent, const Node* use) const;
> 585:   Unique_Node_List _safepoints;

I suggest to move this field to the top of the class before the methods.

-------------

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9961