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

[Emanuel Peter]

On Mon, 13 Nov 2023 16:03:32 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:
> 
>   Fix typo in comment

Apart from comments below this now looks good, thanks for the work :)

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

> 1644: 
> 1645:   // The initial worklist consists of the direct uses
> 1646:   for (DUIterator_Fast kmax, k = fast_outs(kmax); k < kmax; k++)

Add curly braces. Subtle bugs happen because people do not use brackets.

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

> 1652:     if (visited.test_set(use->_idx)) continue;
> 1653:     // Apply callback on boundary nodes
> 1654:     if (is_boundary(use)) callback(use);

We are supposed to always use the block curly brackets.

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

> 1655:     else {
> 1656:       // Not a boundary node, continue search
> 1657:       for (DUIterator_Fast kmax, k = use->fast_outs(kmax); k < kmax; k++)

(optional) You could `test_set` here, then you could avoid pushing them multiple times.

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16429#pullrequestreview-1729140134
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1392173161
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1392163568
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1392165081