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

[Emanuel Peter]

On Thu, 2 Nov 2023 10:09:33 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` for application 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 (on all Oracle-supported platforms)
>> - `tier1`
>> - HotSpot parts of `tier2` and `tier3`
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix typo in visit_uses comment

Changes requested by epeter (Reviewer).

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

> 1134:   // definition of Unique_Node_List
> 1135:   template <typename Callback, typename Bypass>
> 1136:   void visit_uses(Callback callback, Bypass bypass);

You should say in the comment that only "boundary" nodes get a callback.

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

> 1731: // Definition must appear after complete type definition of Unique_Node_List
> 1732: template <typename Callback, typename Bypass>
> 1733: void Node::visit_uses(Callback callback, Bypass bypass) {

Move this to node.cpp, then you don't need to have the `class Unique_Node_List` you added at the top.

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

> 1742:         internals.push(internal_use); // traverse this also
> 1743:       } else {
> 1744:         callback(internal_use);

uses may now have the callback invoked multiple times, for example if we have something like this:


            this
             |
   +---------+--------+
   |                  |
   x                  y
   |                  |
   +--------+ +-------+
            | |
            use

This could not happen with Cast nodes (I think), but now that we generalize the function we should improve the logic, such that callback only gets called once.

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

PR Review: https://git.openjdk.org/jdk/pull/16429#pullrequestreview-1710748405
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1380435531
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1380424281
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1380447427