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

[Roland Westrelin]

On Thu, 19 May 2022 06:03:27 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
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed wrong node input in PhiNode::Ideal

Looks good to me.

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

Marked as reviewed by roland (Reviewer).

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