RFR: 8280696: C2 compilation hits assert(is_dominator(c, n_ctrl)) failed

[Vladimir Kozlov]

On Wed, 18 May 2022 14:34:58 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> We hit an assert when computing early control via `PhaseIdealLoop::compute_early_ctrl` for `1726 AddP` because one of its control inputs `1478 Region` does not dominate current control `1350 Loop` of the AddP:
> ![Screenshot from 2022-05-18 15-10-24](https://user-images.githubusercontent.com/5312595/169046641-2ee94257-0aae-4ddf-bb5f-49dc19b466b3.png)
> 
> I.e., current control of the AddP is incorrect. The problem is that the code in `PhaseIdealLoop::has_local_phi_input` that special cases AddP's only checks control of the Address (and Offset) input, assuming that control of the Base input is consistent.
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/loopopts.cpp#L333-L337
> This is not guaranteed though, leading to the AddP ending up with control that is not dominated by control of its base input.
> 
> As described below, this only reproduces with a very specific sequence of optimizations triggered by replay compilation with `-XX:+StressIGVN` and a fixed seed. I was not able to extract a regression test.
> 
> The fix is to also check control of the Base input when moving the AddP up to a dominating point. For testing purposes, I added an `assert(get_ctrl(m->in(1)) != n_ctrl, "sanity")` without the fix to verify that this change does not affect common cases. It triggers in the failing case but not for any test in tier 1 - 5. In addition, I slightly refactored the code of `PhaseIdealLoop::compute_early_ctrl` and added comments.
> 
> Gory details below.
> 
> Relevant graph after parsing:
> 
>  197  CastPP  ===  1460  60  [[ ... 1724  1725 ]]  #java/io/BufferedReader:NotNull *
>  1459  CastPP  ===  1460  60  [[ ...  1724  1725 ]]  #java/io/BufferedReader:NotNull *
>  1725  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
>  1724  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
>  1726  AddP  === _  1724  1725  41  [[ ... ]]   Oop:java/io/BufferedReader:NotNull+24 *
> 
> `1724 Phi` is then processed by the following code in `PhiNode::Ideal` that replaces its inputs by a cast of the unique input `1730 CastPP`:
> 
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/cfgnode.cpp#L2013-L2016
> 
>  ```
>   197  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1459  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1730  CastPP  ===  1478  60  [[ ...  1724  1724 ]]  #java/io/BufferedReader:NotNull * strong dependency
>  1725  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
>  1724  Phi  ===  1478  1730  1730  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
>  1726  AddP  === _  1724  1725  41  [[ ... ]]   Oop:java/io/BufferedReader:NotNull+24 *
>  ```
> Then `1724 Phi` is replaced by the unique input `1730  CastPP`:
> 
>   197  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1459  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1725  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
>  1730  CastPP  ===  1478  60  [[ ...  1726 ]]  #java/io/BufferedReader:NotNull * strong dependency
>  1726  AddP  === _  1730  1725  41  [[ ... ]]   Oop:java/io/BufferedReader:NotNull+24 *
> 
> Now `1459 CastPP` is replaced by identical `197 CastPP`:
> 
>  197  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1725  Phi  ===  1478  197  197  [[ 1726  1739 ]]  #java/io/BufferedReader:NotNull *
>  1730  CastPP  ===  1478  60  [[ ...  1726 ]]  #java/io/BufferedReader:NotNull * strong dependency
>  1726  AddP  === _  1730  1725  41  [[ ... ]]   Oop:java/io/BufferedReader:NotNull+24 *
> 
> Finally, `1725 Phi` is replaced by unique input `197 CastPP` and the AddP ends up with two casts with different control of the same oop for Base and Address:
> 
>  197  CastPP  ===  1460  60  [[ ...  1725 ]]  #java/io/BufferedReader:NotNull *
>  1730  CastPP  ===  1478  60  [[ ...  1726 ]]  #java/io/BufferedReader:NotNull * strong dependency
>  1726  AddP  === _  1730  197  41  [[ ... ]]   Oop:java/io/BufferedReader:NotNull+24 *
> 
> 
> Looking at the above transformation, the root cause is really the `1730 CastPP` added by `PhiNode::Ideal` which is not needed and prevents the two casts from being merged. Is it worth filing a follow-up enhancement to fix this?
> 
> Thanks,
> Tobias

The question is why we have separate similar `Phi` for Base and Address?:

 1725  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *
 1724  Phi  ===  1478  1459  197  [[ 1726 ]]  #java/io/BufferedReader:NotNull *

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

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