8372824: [lworld] C2 hits "Unexpected argument type" assertion with --enable-preview

[merykitty]

Since I overrode MyValue1::equals in #1929 , TestIntrinsics has failed with -XX:PerMethodSpecTrapLimit=0 -XX:PerMethodTrapLimit=0. The failed test is this one:

// Value class array creation via reflection
@Test
public void test51(int len) {
    Object[] va = (Object[])Array.newInstance(MyValue1.class, len);
    for (int i = 0; i < len; ++i) {
        Asserts.assertEQ(va[i], null);
    }
}

During parsing, Array::newInstance is not inlined, so va is of type Object[] and va[i] is of type Object. Asserts.assertEQ calls va[i].equals(null) which resolves to a CallJavaDynamic with its receiver of type Object, which is passed in as an oop. During incremental inline, It is revealed that va is a MyValue1[] and va[i] is of type MyValue1. This allows the devirtualization of va[i]::equals. Since this is a method call on a value object, the calling convention changes, which leads to the assert because the input is an oop.

I relaxed the assert a little bit to allow changing of the calling convention due to devirtualization.

Please take a look and leave your review, thansk a lot.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8372824: [lworld] C2 hits "Unexpected argument type" assertion with --enable-preview (Bug - P3)

Reviewers

• Tobias Hartmann (@TobiHartmann - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1935/head:pull/1935
$ git checkout pull/1935

Update a local copy of the PR:
$ git checkout pull/1935
$ git pull https://git.openjdk.org/valhalla.git pull/1935/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1935

View PR using the GUI difftool:
$ git pr show -t 1935

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1935.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back qamai! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@merykitty This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8372824: [lworld] C2 hits "Unexpected argument type" assertion with --enable-preview

Reviewed-by: thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the lworld branch:

• 8f8398e: 8374729: [lworld] Enabling CDS crash with UseAltSubstitutabilityMethod
• fa557ef: 8375740: [lworld] javap prints "value class" while class flags indicate ACC_IDENTITY
• a446415: 8375328: [lworld] JNI IsSameObject function doesn't use the alternate substitutability method

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 03: Full - Incremental (f3f230f6)
• 02: Full - Incremental (147fa71e)
• 01: Full - Incremental (5000c519)
• 00: Full (b9994691)

[TobiHartmann]

So this is post-parse call-devirtualization, i.e. JDK-8257211, right? Could we assert call->generator()->is_virtual_late_inline() here?

[merykitty]

Unfortunately, the CallNode here does not have a generator, because it is created just above. Checking is_late_inline does not work either, because we are too deep handling now. LateInlineVirtualCallGenerator::do_late_inline_check calls into Compile::call_generator to find the replacement. This method, after deciding that it does not want to inline the callee, will create a PredictedCallGenerator with the fast path being a DirectCallGenerator. And this DirectCallGenerator is the one calling the method we are in, and this subclass of CallGenerator does not return true for is_late_inline().

[TobiHartmann]

Ah, too bad. What about Compile::strength_reduction that @dafedafe introduced with #1768? I think it would just be nice to have a strong assert here because these kind of asserts helped in the past to catch cases where we failed to scalarize early.

[merykitty]

Thanks for the pointer, I have added some asserts that we are during call devirtualization, not during parsing.

[TobiHartmann]

Great! That looks good to me.