Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263024: Convert Valhalla tests using the old framework to the new framework #457

Closed
wants to merge 4 commits into from

Conversation

katyapav
Copy link
Member

@katyapav katyapav commented Jun 21, 2021

This PR converts compiler/valhalla/inlinetypes tests based on old Valhalla/IR test framework to new one
which was recently integrated by Christian as part of JDK-8254129.
The tests which were we decided to convert are basically the ones which extends InlineTypeTest class.
The rest of compiler/valhalla/inlinetypes tests were agreed (with Christian and Tobias) to keep untouched
as they are in most cases regression tests.

The conversion rules/approach is pretty well described in the bug's description section (see JDK-8263024).
A detailed description of new IR test framework could found in test/hotspot/jtreg/compiler/lib/ir_framework/README.md.

Testing:
Ran compiler/valhalla/inlinetypes in all the configurations used in hs-tier1-9.
There 2 failed converted tests:
compiler/valhalla/inlinetypes//TestNullableArrays.java - new bug JDK-8269070 filed
compiler/valhalla/inlinetypes/TestIntrinsics.java - know issue tracked by JDK-8239003

Big thanks to Christian and Tobias for advice and help during this tests conversion.

Please review the changes.

Thanks,
Katya


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8263024: Convert Valhalla tests using the old framework to the new framework

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/457/head:pull/457
$ git checkout pull/457

Update a local copy of the PR:
$ git checkout pull/457
$ git pull https://git.openjdk.java.net/valhalla pull/457/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 457

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/457.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 21, 2021

👋 Welcome back epavlova! 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
Copy link

openjdk bot commented Jun 21, 2021

@katyapav 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:

8263024: Convert Valhalla tests using the old framework to the new framework

Reviewed-by: thartmann, chagedorn

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jun 21, 2021
@katyapav
Copy link
Member Author

katyapav commented Jun 21, 2021

/reviewer @chhagedorn

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

  • /reviewer credit @openjdk-bot
  • /reviewer credit duke
  • /reviewer credit @user1 @user2

@katyapav
Copy link
Member Author

katyapav commented Jun 21, 2021

/label hotspot-compiler

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav The label hotspot is not a valid label. These labels are valid:

@katyapav katyapav changed the title Converted valhalla/inlinetypes to new IR framework Convert valhalla/inlinetypes tests to new IR framework Jun 21, 2021
@katyapav
Copy link
Member Author

katyapav commented Jun 21, 2021

/label add hotspot

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav The label hotspot is not a valid label. These labels are valid:

@katyapav
Copy link
Member Author

katyapav commented Jun 21, 2021

/label add hotspot-compiler

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav The label hotspot-compiler is not a valid label. These labels are valid:

@katyapav katyapav marked this pull request as ready for review Jun 21, 2021
@katyapav
Copy link
Member Author

katyapav commented Jun 21, 2021

/label add hotspot-compiler
/label add hotspot

@openjdk openjdk bot added the rfr label Jun 21, 2021
@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav The label hotspot-compiler is not a valid label. These labels are valid:

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@katyapav The label hotspot is not a valid label. These labels are valid:

@mlbridge
Copy link

mlbridge bot commented Jun 21, 2021

Webrevs

@veresov
Copy link
Contributor

veresov commented Jun 21, 2021

/label hotspot-compiler

@openjdk
Copy link

openjdk bot commented Jun 21, 2021

@veresov Only the PR author and project Committers are allowed to modify labels on a PR.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks a lot for taking care of this, Katya! The changes look good to me, just TestIntrinsics.java needs merging.

Please link JDK-8263024 to the PR.

Best regards,
Tobias

@openjdk
Copy link

openjdk bot commented Jul 12, 2021

@katyapav this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8263024
git fetch https://git.openjdk.java.net/valhalla lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

@katyapav katyapav changed the title Convert valhalla/inlinetypes tests to new IR framework 8263024: Convert valhalla/inlinetypes tests to new IR framework Jul 13, 2021
@katyapav katyapav changed the title 8263024: Convert valhalla/inlinetypes tests to new IR framework 8263024: Convert Valhalla tests using the old framework to the new framework Jul 13, 2021
@katyapav
Copy link
Member Author

katyapav commented Jul 13, 2021

@TobiHartmann, thanks for your review!
I did merge TestIntrinsics.java and link the bug.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks, that looks good to me.

Copy link
Member

@chhagedorn chhagedorn left a comment

Thanks a lot for your help to do the conversions! They look good overall. There are only some minor things, mostly about code style.

@Warmup(10000)
@Test(failOn = SUBSTITUTABILITY_TEST)
@Test
@IR(failOn = {SUBSTITUTABILITY_TEST})
Copy link
Member

@chhagedorn chhagedorn Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified to failOn = SUBSTITUTABILITY_TEST. There are some more of them below which could be simplified as well.

Copy link
Member Author

@katyapav katyapav Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually specially kept {} so it will be more straightforward to add more nodes if needed. But I can remove {} if you prefer simplified version.

long result = test2();
Asserts.assertEQ(result, hash());
}

// Test receiving an inline type array from the interpreter,
// updating its elements in a loop and computing a hash.
@Test(failOn = ALLOCA)
@Test
@IR(failOn = {ALLOCA})
Copy link
Member

@chhagedorn chhagedorn Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified to failOn = ALLOCA.

Copy link
Member Author

@katyapav katyapav Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see previous comment

@@ -2556,13 +2563,14 @@ public void test96_verifier(boolean warmup) {
}

// Test loads from vararg arrays
@Test(failOn = LOAD_UNKNOWN_INLINE)
@Test
@IR(failOn = {LOAD_UNKNOWN_INLINE})
Copy link
Member

@chhagedorn chhagedorn Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified to failOn = LOAD_UNKNOWN_INLINE.

Copy link
Member Author

@katyapav katyapav Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see previous comment

public void test141_verifier(boolean warmup) {
@Run(test = "test141")
@Warmup(0)
public void test141_verifier() {
int result = test141();
Asserts.assertEquals(result, 0);
}
Copy link
Member

@chhagedorn chhagedorn Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just seen that there is one more test below (test142) which was not converted, yet.

Copy link
Member Author

@katyapav katyapav Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :), converted

@katyapav
Copy link
Member Author

katyapav commented Jul 14, 2021

I did rerun compiler/valhalla/inlinetypes in all the configurations used in hs-tier1-9
and only compiler/valhalla/inlinetypes/TestIntrinsics.java failed which is known issue tracked by JDK-8239003

Copy link
Member

@chhagedorn chhagedorn left a comment

Thanks for doing the changes!

@katyapav
Copy link
Member Author

katyapav commented Jul 15, 2021

/sponsor @chhagedorn

@openjdk
Copy link

openjdk bot commented Jul 15, 2021

@katyapav Only Committers are allowed to sponsor changes.

@katyapav
Copy link
Member Author

katyapav commented Jul 15, 2021

/integrate

@katyapav
Copy link
Member Author

katyapav commented Jul 15, 2021

/sponsor @TobiHartmann

@openjdk openjdk bot added the sponsor label Jul 15, 2021
@openjdk
Copy link

openjdk bot commented Jul 15, 2021

@katyapav
Your change (at version 059261d) is now ready to be sponsored by a Committer.

@openjdk
Copy link

openjdk bot commented Jul 15, 2021

@katyapav Only Committers are allowed to sponsor changes.

@MrSimms
Copy link
Member

MrSimms commented Jul 15, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Jul 15, 2021

Going to push as commit aa84c4e.

@openjdk
Copy link

openjdk bot commented Jul 15, 2021

@MrSimms @katyapav Pushed as commit aa84c4e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants