RFR: 8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance

[Vladimir Kozlov]

On Fri, 30 Oct 2020 11:28:10 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> The dominance failures start to occur after the fix for [JDK-8249749](https://bugs.openjdk.java.net/browse/JDK-8249749) which enabled the method `SWPointer::scaled_iv_plus_offset` to call itself recursively and walk the graph to match more instead of stopping immediately (no recursion):
> https://github.com/openjdk/jdk/commit/092389e3c91822b1e3f56f203cb7b90e84673f8e#diff-8f29dd005a0f949d108687dabb7379c73dfd85cd782da453509dc9b6cb8c9f81L3789-R3812
> 
> We check in `SWPointer::offset_plus_k` if a node is invariant and if so then we choose it as invariant. However, we now have cases in the Renaissance benchmarks where we select an invariant that is pinned to a `CastIINode` between the main and pre loop. An example is shown in the attached image. 5913 SubI is found as an invariant with the improved recursive search enabled by JDK-8249749. The control of 5913 SubI (with `get_ctrl`) is 5298 CastII. The problem is now that we use the invariant 5913 SubI in the pre loop limit check of  5281 CountedLoopEnd (done in `SuperWord::align_initial_loop_index`) because we assume that since the invariant is not part of the main loop, it can float into the pre loop. But this is prevented by 5298 CastII. This results in the dominance assertion failure when checking if the earliest control of 5270 Bool in the pre loop (5297 IfTrue because of 5913 SubI that is used by 5270 Bool) dominates the LCA of 5270 Bool (the pre loop header node).
> 
> My suggestion is to improve the invariant check in `SWPointer::offset_plus_k` to also check if the found invariant is not dominated by the pre loop end node. Repeated testing of the RenaissanceStressTest has not resulted in any dominance failures anymore.
> ![dominance_failure](https://user-images.githubusercontent.com/17833009/97696669-3752d200-1aa6-11eb-9a42-2e36550e2b8b.png)

I would also suggest to run locally next jtreg command and compare number of created vector nodes to make sure your changes did not affect common cases:

`$ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server  -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log`

`$ grep "new Vector node:" new_vects.log|wc`

src/hotspot/share/opto/superword.cpp line 3443:

> 3441:   assert(lp()->is_main_loop(), "");
> 3442:   CountedLoopEndNode* pre_end = cached_pre_loop_end();
> 3443:   assert(get_pre_loop_end(lp()), "pre loop end must still be found");

You already have similar assert check inside cached_pre_loop_end().

src/hotspot/share/opto/superword.cpp line 921:

> 919:   }
> 920:   CountedLoopEndNode* pre_end = cached_pre_loop_end();
> 921:   assert(get_pre_loop_end(lp()), "pre loop end must still be found");

You already have similar assert check inside cached_pre_loop_end().

src/hotspot/share/opto/superword.hpp line 345:

> 343: #ifdef ASSERT
> 344:     Node* pre_end = get_pre_loop_end(_lp);
> 345:     assert(_lp != NULL && (pre_end == NULL || pre_end == _cached_pre_loop_end) , "real CLE either not found anymore (NULL) or unchanged");

Check _lp != NULL should be before it use in previous line.

src/hotspot/share/opto/superword.hpp line 346:

> 344:     Node* pre_end = get_pre_loop_end(_lp);
> 345:     assert(_lp != NULL && (pre_end == NULL || pre_end == _cached_pre_loop_end) , "real CLE either not found anymore (NULL) or unchanged");
> 346:     assert(_cached_pre_loop_end != NULL, "should be set when fetched");

This should be very first assert in this method.

src/hotspot/share/opto/superword.hpp line 354:

> 352:   }
> 353: 
> 354:   int      iv_stride()             { return lp()->stride_con(); }

Can you move this method right after set_lp() as originally?

src/hotspot/share/opto/superword.cpp line 3801:

> 3799: }
> 3800: 
> 3801: bool SWPointer::invariant_not_dominated_by_pre_loop_end(Node* n) {

I think we should have only one invariant() method which does this additional check.
And have separate method is_main_loop_member() where currently we check !invariant().

src/hotspot/share/opto/superword.cpp line 3810:

> 3808:     // This happens, for example, when n_c is a CastII node that prevents data nodes to flow above the main loop and into
> 3809:     // the pre loop. Use the cached version as the real pre loop end might not be found anymore with get_pre_loop_end().
> 3810:     return !phase()->is_dominator(_slp->cached_pre_loop_end(), n_c);

I think it is not enough. We don't want invariant be inside pre-loop.
Invariant should be node outside original loop (before splitting and unrolling).
We should check that n_c dominates pre-loop head.
What do you think?

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

Changes requested by kvn (Reviewer).

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