RFR: 8275330: C2: assert(n->is_Root() || n->is_Region() || n->is_Phi() || n->is_MachMerge() || def_block->dominates(block)) failed: uses must be dominated by definitions

[Christian Hagedorn]

On Wed, 17 Nov 2021 12:16:32 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> This is similar to previous bugs where:
> 
> - a cast/conv node captures a narrow type in a loop body because of a
>   range check,
> 
> - the range check is optimized out of the loop, pre/main/post loop are
>   created
> 
> - overunrolling causes the main loop to become unreachable (the range
>   check, if still in the main loop, would fail), the cast transforms to
>   top but c2 can't optimize the loop out
> 
> This was fixed by adding predicates above the main loop. With this
> particular bug, the cast node is in the post loop. The fix I propose
> is to also add predicates above the post loop. There are a few
> locations in the code that cause a post loop to be added: either the
> initial post loop or some other post loops for vectorization
> support. I think the new predicates are needed in a all cases. To be
> able to add predicates at these different points in the optimization
> process, the new predicates are copied from the main loop predicates.
> 
> I also delayed folding of Opaque4 nodes to macro expansion rather than
> post loop opts igvn. The reason for that is that I believe there's a
> risk that an Opaque4 is removed (that is replaced by its input 2)
> before its input 1 has a chance to constant fold. That wouldn't happen
> with a debug build because we leave the tests in (that is replace the
> Opaque4 node by its input 1) so that corner case is not properly
> tested currently. The reason for leaving the tests in was to sanity
> check that the tests are indeed correct.

Otherwise, the fix looks reasonable to me! When I was fixing related bugs before, I found myself wondering if the post loop does not need these predicates as well - turns out now it does.

src/hotspot/share/opto/loopTransform.cpp line 1549:

> 1547:   CountedLoopNode *post_head = NULL;
> 1548:   Node* post_incr = incr;
> 1549:   Node *main_exit = insert_post_loop(loop, old_new, main_head, main_end, post_incr, limit, post_head);

Could also be updated: `Node *main_exit` -> `Node* main_exit`.

src/hotspot/share/opto/loopTransform.cpp line 1923:

> 1921:   Node* castii = cast_incr_before_loop(zer_opaq->in(1), zer_taken, post_head);
> 1922:   assert(castii != NULL, "no castII inserted");
> 1923:   incr = castii;

You could directly assign `incr` on L1921 instead of using the additional `castii` variable.

src/hotspot/share/opto/loopTransform.cpp line 1978:

> 1976: }
> 1977: 
> 1978: void PhaseIdealLoop::insert_post_loop_skeleton_predicates(LoopNode* main_loop_head, CountedLoopNode* post_loop_head, Node* init, Node* stride) {

Maybe this could be renamed to `copy_skeleton_predicates_to_post_loop()` to be consistent with the method `copy_skeleton_predicates_to_main_loop()`?

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6429