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

[Emanuel Peter]

On Tue, 7 Nov 2023 07:59:30 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:
> 
>   Add UnlockDiagnosticVMOptions to allow running new test on product build

Changes requested by epeter (Reviewer).

Sorry for all the nit-picks 🙈

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

> 1133:   // reaching a boundary node, defined by is_boundary.
> 1134:   // Note: the function definition appears after the complete type definition
> 1135:   // of Unique_Node_List.

Nit: generally we try to make the comments in "block" style. You could also make the lines a bit wider.

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

> 1140:   // Pattern: this (-> ConstraintCast)* -> non_cast
> 1141:   // In other words: find all non_cast nodes such that
> 1142:   // non_cast->uncast() == this.

same here

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

> 1737:   // never pop anything from the worklist, since that could result in applying
> 1738:   // the callback for a use more than once (if it is at some point readded to
> 1739:   // the worklist).

This is block style, but it would be ok to make lines longer

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

> 1742:   for (DUIterator_Fast kmax, k = fast_outs(kmax); k < kmax; k++) {
> 1743:     worklist.push(fast_out(k));
> 1744:   }

An alternative could be to just push "this", and then in the loop: `if (use == this) { continue; }`
Would just save the code duplication.

test/hotspot/jtreg/compiler/ccp/TestShiftConvertAndNotification.java line 41:

> 39:  * @summary Test CCP notification for value update of AndL through LShiftI and
> 40:  *          ConvI2L (reduced set of flags).
> 41:  * @run main/othervm compiler.ccp.TestShiftConvertAndNotification

Suggestion:

 * @run driver compiler.ccp.TestShiftConvertAndNotification

If you have no flags, you don't need to spawn a new VM. `driver` executes the tests in the driver vm.

https://openjdk.org/jtreg/faq.html#what-are-the-agentvm-and-othervm-modes

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

PR Review: https://git.openjdk.org/jdk/pull/16429#pullrequestreview-1719761416
PR Comment: https://git.openjdk.org/jdk/pull/16429#issuecomment-1801393503
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1386248426
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1386248986
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1386253401
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1386253424
PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1386256462