RFR: 8309268: C2: "assert(in_bb(n)) failed: must be" after JDK-8306302

[Roberto Castañeda Lozano]

On Thu, 1 Jun 2023 16:19:41 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> This is the fix to a regression caused in the CMoveV fix JDK-8306302.
> 
> I had implicitly assumed that all `Cmp` in the loop also have their `in(1)` inside the loop (`in_bb`). This is not always true, and hence we hit the assert.
> 
> **Solution**
> 
> However, we know that at least one of the two inputs of a `Cmp` must also be in the loop, else the `Cmp` would float outside the loop. So if `in(1)` is not in the loop then we can just pick `in(2)` for the `velt_type`.
> 
> **Testing**
> I added 2 regression tests that were provided in the bug, and also extended `TestVectorConditionalMove.java` (though it is currently problemlisted because of an IR framework bug). This extension also triggered the assert, and now properly vectorizes.
> 
> I tested up to tier6 and stress testing. I ran the tests both with `TestVectorConditionalMove` problemlisted and without the problemlisting.
> **Running... but so far all good**

Looks good, I just have a few minor suggestions.

src/hotspot/share/opto/superword.cpp line 3710:

> 3708:     }
> 3709:     if (nn->is_Cmp() && nn->in(0) == nullptr) {
> 3710:       // One of the inputs must be in_bb, pick that velt_type

Could you turn this comment into an assertion?

test/hotspot/jtreg/compiler/c2/irTests/TestVectorConditionalMove.java line 1:

> 1: /*

Please add an Oracle copyright entry to the header of this file, see e.g. https://github.com/openjdk/jdk/blob/cb1e5e3f0fb499ce3420a57a08fb9ec434809d13/test/hotspot/jtreg/compiler/c2/irTests/TestSuperwordFailsUnrolling.java#L2-L3

test/hotspot/jtreg/compiler/c2/irTests/TestVectorConditionalMove.java line 873:

> 871:             Asserts.assertEquals(rD[i], cmoveDGTforD(aD[i], bD[i], cD[i], dD[i]));
> 872:         }
> 873: 

Nit: remove one of the empty lines.

test/hotspot/jtreg/compiler/c2/irTests/TestVectorConditionalMove.java line 875:

> 873: 
> 874: 
> 875:         // Use some constaints in the comparison

Suggestion:

        // Use some constants in the comparison

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

Marked as reviewed by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14268#pullrequestreview-1457334808
PR Review Comment: https://git.openjdk.org/jdk/pull/14268#discussion_r1214331074
PR Review Comment: https://git.openjdk.org/jdk/pull/14268#discussion_r1214337644
PR Review Comment: https://git.openjdk.org/jdk/pull/14268#discussion_r1214333569
PR Review Comment: https://git.openjdk.org/jdk/pull/14268#discussion_r1214333159