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

[Vladimir Kozlov]

On Thu, 19 Nov 2020 17:06:20 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Remove _pre_loop_head field
>  - Update comments and invariant selection in offset_plus_k
>  - Check dominance with pre loop head instead of tail
>  - 8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance

Unfortunately with forced push I can't see incremental update.
Based on last update comments changes are reasonable.

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

Marked as reviewed by kvn (Reviewer).

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