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

8266086: [lworld][lw3] C1 produces incorrect code when GlobalValueNumbering is used #395

Closed
wants to merge 2 commits into from

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented Apr 28, 2021

Please review this fix in C1 GlobalValueNumbering.
The problem is that C1 doesn't track that a flattened field has been updated when it writes the individual values of this flattened field. The proposed fix is to record the enclosing flattened field with the StoreField node of each individual field, and use this information when the GlobalValueNumbering processes those nodes to kill the flattened field in the ValueMap.

Tested locally (Linux 64) with hotspot_valhalla (including new unit test) and jdk_valhalla test suites.

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8266086: [lworld][lw3] C1 produces incorrect code when GlobalValueNumbering is used

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 395

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 28, 2021

👋 Welcome back fparain! 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 Apr 28, 2021

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

8266086: [lworld][lw3] C1 produces incorrect code when GlobalValueNumbering is used

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 2 new commits pushed to the lworld branch:

  • 8dc4430: 8265726: [lworld] C2 compilation fails with assert "uses must be dominated by definitions"
  • b65c2b1: 8265376: Prepare for javac change to do the member translation as described in SoV

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
Copy link

mlbridge bot commented Apr 28, 2021

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Good catch! I've added some comments.

@@ -0,0 +1,72 @@
/*
* Copyright (c) 2020, 2020, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Copyright should be 2021.

/*
* @test
* @summary Test value numbering behaves correctly with flattened fields
* @library /testlibrary /test/lib /compiler/whitebox /
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Whitebox is not used.

* @test
* @summary Test value numbering behaves correctly with flattened fields
* @library /testlibrary /test/lib /compiler/whitebox /
* @compile TestC1ValueNumbering.java
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Not required.

* @summary Test value numbering behaves correctly with flattened fields
* @library /testlibrary /test/lib /compiler/whitebox /
* @compile TestC1ValueNumbering.java
* @run main/othervm -Xcomp -XX:TieredStopAtLevel=1 -ea -XX:+UseGlobalValueNumbering
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

UseGlobalValueNumbering is true by default.

*/

public class TestC1ValueNumbering {
static primitive class Point {
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

4x whitespace indentation should be used for Java code.

@@ -148,6 +148,9 @@ class ValueNumberingVisitor: public InstructionVisitor {
kill_memory();
} else {
kill_field(x->field(), x->needs_patching());
if (x->enclosing_field() != NULL) {
kill_field(x->enclosing_field(), true);
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Why do you need all_offsets == true? The offset of the enclosing field should always be known, right?

@fparain
Copy link
Collaborator Author

fparain commented Apr 29, 2021

Hi Tobias,

Thank you for the review.
Most of the issues you spotted have been fixed on the latest commit.
Regarding the all_offset == true issue, it is needed to invalidate all field entries in the ValueMap. We would like to only invalidate the entries of the nested fields related to the flattened field. But there's no infrastructure yet to easily identity which fields are related to a given flattened field. So the conservative approach is to invalidate all field entries.

Regards,

Fred

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for updating, looks good to me!

Best regards,
Tobias

@fparain
Copy link
Collaborator Author

fparain commented Apr 29, 2021

/integrate

@openjdk openjdk bot closed this Apr 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 29, 2021
@openjdk
Copy link

openjdk bot commented Apr 29, 2021

@fparain Since your change was applied there have been 2 commits pushed to the lworld branch:

  • 8dc4430: 8265726: [lworld] C2 compilation fails with assert "uses must be dominated by definitions"
  • b65c2b1: 8265376: Prepare for javac change to do the member translation as described in SoV

Your commit was automatically rebased without conflicts.

Pushed as commit a7ab3bb.

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

@fparain fparain deleted the c1_valuenumbering branch Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants