[jdk19] Integrated: 8288683: C2: And node gets wrong type due to not adding it back to the worklist in CCP

[Christian Hagedorn]

On Fri, 24 Jun 2022 08:54:03 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> [JDK-8277850](https://bugs.openjdk.org/browse/JDK-8277850) added some new optimizations in `AndI/L::Value()` to optimize patterns similar to `(v << 2) & 3` which can directly be replaced by zero if the mask and the shifted value are bitwise disjoint. To do that, we look at the type of the shift value of the `LShift` input (right-hand side input):
> https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/bdf9902f753b71f30be8e1634fc361a5c7d8d8ec/src/hotspot/share/opto/mulnode.cpp*L1752-L1765__;Iw!!ACWV5N9M2RV99hQ!Lp6rj5qfngPeLfWDyNzIzP2mvHVplXhoRKOwAOJ4GmlbbgfgYtDG2D33bvqbWoCVwMsbLC10t7nPxFsfLlgG3scAmckJ_E4hZg$ 
> 
> The optimization as such works fine but there is a problem in CCP. After calling `Value()` for a node in CCP, we generally only add the direct users of it back to the worklist if the type changed:
> https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/bdf9902f753b71f30be8e1634fc361a5c7d8d8ec/src/hotspot/share/opto/phaseX.cpp*L1812-L1814__;Iw!!ACWV5N9M2RV99hQ!Lp6rj5qfngPeLfWDyNzIzP2mvHVplXhoRKOwAOJ4GmlbbgfgYtDG2D33bvqbWoCVwMsbLC10t7nPxFsfLlgG3scAmcmRqa0Gcg$ 
> 
> We special case some nodes where we need to add additional nodes (grandchildren or even further down) back to the worklist to not miss updating them, for example, the `Phis` when the use is a `Region`:
> https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/bdf9902f753b71f30be8e1634fc361a5c7d8d8ec/src/hotspot/share/opto/phaseX.cpp*L1789-L1796__;Iw!!ACWV5N9M2RV99hQ!Lp6rj5qfngPeLfWDyNzIzP2mvHVplXhoRKOwAOJ4GmlbbgfgYtDG2D33bvqbWoCVwMsbLC10t7nPxFsfLlgG3scAmclv68qwqA$ 
> 
> However, we miss to special case an `LShift` use if the shift value (right-hand side input of the `LShift`) changed. We should add all `AndI/L` nodes back to the worklist to account for the `AndI/L::Value()` optimization. Not doing so can result in a wrong execution as shown with the testcase. We have the following nodes:
> ![Screenshot from 2022-06-24 10-28-41](https://urldefense.com/v3/__https://user-images.githubusercontent.com/17833009/175496296-4280e26b-6f2f-4ddc-b164-b9e887a5d437.png__;!!ACWV5N9M2RV99hQ!Lp6rj5qfngPeLfWDyNzIzP2mvHVplXhoRKOwAOJ4GmlbbgfgYtDG2D33bvqbWoCVwMsbLC10t7nPxFsfLlgG3scAmcmuFB5b2w$ )
> 
> The `LShiftI` node gets `int` as type (i.e. bottom) and is not put back on the worklist again since the type cannot improve anymore. Afterwards, we process the `AndI` node and call `AndI::Value()`. At this point, the phi node still has the temporary type `int:62`. We apply the optimization that the shifted number and the mask are bitwise disjoint and we set the type of the `AndI` node to `int:0`. When later reapplying `Phi::Value()` for the phi node, we correct the type to `int:62..69` and try to push the `LShiftI` node use back to the worklist. Since its type is `int`, we do not add it again. At this point, `AndI` is not on the CCP worklist anymore and neither will we push the `AndI` node to it again. We miss to reapply `AndI::Value()` and correct the now wrong `Value()` optimization. We keep `int:0` as type and replace the `AndI` node by constant zero - leading to a wrong execution.
> 
> Special casing `LShift` -> `AndNodes` in CCP fixes the problem to make sure we reapply `AndI/L::Value()` again. I've applied some more refactorings and comment improvements but since this fix is for JDK 19, I've decided to separate them into an RFE ([JDK-8289051](https://bugs.openjdk.org/browse/JDK-8289051)) to reduce the risk.
> 
> At some point, we should add some additional verification to find missed `Value()` calls in CCP to avoid similar problems in the future (see [JDK-8257197](https://bugs.openjdk.org/browse/JDK-8257197)).
> 
> Thanks,
> Christian

This pull request has now been integrated.

Changeset: 784a0f04
Author:    Christian Hagedorn <chagedorn at openjdk.org>
URL:       https://git.openjdk.org/jdk19/commit/784a0f049665afde4723942e641a10a1d7675f7a
Stats:     110 lines in 3 files changed: 110 ins; 0 del; 0 mod

8288683: C2: And node gets wrong type due to not adding it back to the worklist in CCP

Reviewed-by: roland, thartmann

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

PR: https://git.openjdk.org/jdk19/pull/65