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

[Tobias Hartmann]

On Wed, 4 Nov 2020 13:31:06 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)
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update comments and invariant selection in offset_plus_k

Otherwise looks good to me.

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

> 71:   _lpt(NULL),                             // loop tree node
> 72:   _lp(NULL),                              // CountedLoopNode
> 73:   _pre_loop_head(NULL),                   // Pre loop CountedLoopNode

Is this field really needed? Can't we just retrieve the pre-loop head via `_pre_loop_end->loopnode()`?

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

Marked as reviewed by thartmann (Reviewer).

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