RFR: 8313672: C2: PhaseCCP does not correctly track analysis dependencies involving shift, convert, and mask [v13]

[Roberto Castañeda Lozano]

On Fri, 10 Nov 2023 15:36:16 GMT, Daniel Lundén <duke at openjdk.org> wrote:

>> This changeset fixes an issue where the `PhaseCCP` analysis stopped before fixpoint due to `Value`-updates for `AndL` nodes involving shift, convert, and mask (`MulNode::AndIL_shift_and_mask_is_always_zero`). Such updates did not previously repopulate the `PhaseCCP` worklist correctly.
>> 
>> Changes:
>> - Fix the issue by adding the missing pattern to `PhaseCCP::push_and`.
>> - Generalize `visit_uncasted_uses` to `visit_uses` to enable the functionality required in `PhaseCCP::push_and`.
>> - Update the `assert` error message that reported the issue to better describe the problem. The `PhaseCCP` analysis is optimistic, and the previous error message seemed to indicate the opposite ("Missed optimization opportunity"). If the assert triggers, it could be that the analysis result leads to unsound transformations later on.
>> - Add a regression test
>> 
>> Result after `PhaseCCP` analysis (without fix, note `long:0` for node 116):
>> ![before](https://github.com/openjdk/jdk/assets/4222397/3f2343da-8573-464d-9352-a147d0faeab8)
>> 
>> Result after `PhaseCCP` analysis (with fix, note `long` for node 116):
>> ![after](https://github.com/openjdk/jdk/assets/4222397/6bf44964-cff8-49db-9825-4a4667f4e769)
>> 
>> ### Testing
>> `tier1`, `tier2`, `tier3`, `tier4`, `tier5` (windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64)
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make visit_uses and visit_uncasted_uses const member functions

Thanks for addressing my feedback! I have a couple more comments.

src/hotspot/share/opto/node.hpp line 1145:

> 1143:      visit_uses(callback, [](Node* n){ return !n->is_ConstraintCast(); });
> 1144:   }
> 1145: 

`visit_uncasted_uses` is only used once, I think it would be better to inline it in `PhaseIterGVN::add_users_to_worklist()`, for simplicity and also for symmetry with `PhaseCCP::push_and`.

src/hotspot/share/opto/node.hpp line 1738:

> 1736:   // the callback for a use more than once (if it is at some point readded to
> 1737:   // the worklist).
> 1738:   Unique_Node_List worklist;

I think it is preferable, for simplicity and for memory efficiency, to use a queue/stack with an explicit `VectorSet` (typically called `"visited"`) idiom for the traversal.

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16429#pullrequestreview-1726879124
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1390835839
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1390844633