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

8271589: fatal error with variable shift count integer rotate operation. #4956

Closed
wants to merge 3 commits into from

Conversation

@jatin-bhateja
Copy link
Member

@jatin-bhateja jatin-bhateja commented Aug 2, 2021

Problem seen on targets which do not support variable vector shifts, in such a case vector rotation node inferred by auto-vectorizer are not disintegrable into LeftShift/RightShift and Or operations.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271589: fatal error with variable shift count integer rotate operation.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4956

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 2, 2021

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

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 2, 2021

/label hotspot-compiler-dev

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 2021

@jatin-bhateja
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 2, 2021

Webrevs

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

I assume TestLongVectRotate.java should be also updated.

And you need add cases for Long type vector shifts if they can be generated.

Loading

@@ -2488,7 +2488,6 @@ void SuperWord::output() {
} else if (VectorNode::is_scalar_rotate(n)) {
Node* in1 = low_adr->in(1);
Node* in2 = p->at(0)->in(2);
assert(in2->bottom_type()->isa_int(), "Shift must always be an int value");
Copy link
Contributor

@vnkozlov vnkozlov Aug 2, 2021

Choose a reason for hiding this comment

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

Why you removed the assert? What other types can be seen here?
The following code (specifically in2->get_int()) will fail if it is not Integer type.

Loading

Copy link
Member Author

@jatin-bhateja jatin-bhateja Aug 2, 2021

Choose a reason for hiding this comment

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

At this point in code, the shift is either an int value or a vector of an int value. If it's a vector !in2->is_Con() will make the condition true and no need to check for get_int due to short-circuiting.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Aug 2, 2021

Choose a reason for hiding this comment

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

Okay. IT should be fine because get_int() has own assert.

Loading

} else {
// Variable shift case.
} else if (VectorNode::is_invariant_vector(cnt)) {
// Scalar variable shift, handle replicates generated by auto vectorizer.
assert(VectorNode::is_invariant_vector(cnt), "Broadcast expected");
Copy link
Contributor

@vnkozlov vnkozlov Aug 2, 2021

Choose a reason for hiding this comment

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

This assert is not needed anymore since you already checked for this case.

Loading

Copy link
Member Author

@jatin-bhateja jatin-bhateja Aug 2, 2021

Choose a reason for hiding this comment

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

Will remove this before checking in the final version of the patch.

Loading

cnt = cnt->in(1);
} else {
assert(cnt->bottom_type()->isa_long() &&
cnt->bottom_type()->is_long()->is_con(), "Long constant expected");
Copy link
Contributor

@vnkozlov vnkozlov Aug 2, 2021

Choose a reason for hiding this comment

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

Is this case from Vector API? I think in Java we can have only Int type for shifts.

Loading

Copy link
Member Author

@jatin-bhateja jatin-bhateja Aug 2, 2021

Choose a reason for hiding this comment

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

Is this case from Vector API? I think in Java we can have only Int type for shifts.

This is to handle long rotates, SLP will insert a ConvI2L before broadcasting shift value into a long vector. In case of constant int value, ConvI2L::Value will create a constant of TypeLong and which is what is being handled here.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Aug 2, 2021

Choose a reason for hiding this comment

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

Got it.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 2, 2021

I assume TestLongVectRotate.java should be also updated.

And you need add cases for Long type vector shifts if they can be generated.

Long rotates patterns with variable shifts i.e. Long.rotateLeft/Right(a[i], int_b[i]/ (int) long_b[i]) are not auto-vectorized currently.
Problem is only seen with rotation over integers. I have kept the changes minimal as suggested earlier.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 2, 2021

Hi @vnkozlov, I have responded to your individual comments, please let me know if there are more comments.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 2, 2021

I assume TestLongVectRotate.java should be also updated.
And you need add cases for Long type vector shifts if they can be generated.

Long rotates patterns with variable shifts i.e. Long.rotateLeft/Right(a[i], int_b[i]/ (int) long_b[i]) are not auto-vectorized currently.
Problem is only seen with rotation over integers. I have kept the changes minimal as suggested earlier.

You said in an other comment:
"This is to handle long rotates, SLP will insert a ConvI2L before broadcasting shift value into a long vector."

Is it really a big change for test to cover this case? This is new code in your changes which needs test coverage I think.

I will start testing current changes (with removed assert(is_invariant_vector()) we agreed on).

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 2, 2021

Testing passed clean.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 3, 2021

I assume TestLongVectRotate.java should be also updated.
And you need add cases for Long type vector shifts if they can be generated.

Long rotates patterns with variable shifts i.e. Long.rotateLeft/Right(a[i], int_b[i]/ (int) long_b[i]) are not auto-vectorized currently.
Problem is only seen with rotation over integers. I have kept the changes minimal as suggested earlier.

You said in an other comment:
"This is to handle long rotates, SLP will insert a ConvI2L before broadcasting shift value into a long vector."

Yes this comment is valid for Long.rotateLeft/Right(a[i], SHIFT/IMM_SHIFT) patterns.

Is it really a big change for test to cover this case? This is new code in your changes which needs test coverage I think.

I will start testing current changes (with removed assert(is_invariant_vector()) we agreed on).

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 3, 2021

@jatin-bhateja I don't understand why you don't want to add testing for Long.rotateLeft/Right(a[i], SHIFT/IMM_SHIFT) .
Yes, this bug failed only for Int vectors. But you added code for Longs which have to be verified.

And you need second review.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 3, 2021

@jatin-bhateja I don't understand why you don't want to add testing for Long.rotateLeft/Right(a[i], SHIFT/IMM_SHIFT) .
Yes, this bug failed only for Int vectors. But you added code for Longs which have to be verified.

And you need second review.

Hi @vnkozlov ,
My apologies for not mentioning earlier that such test patterns (Long.rotateLeft/Right(a[i], scalar_shift/imm_shift) are already present in jdk/test/hotspot/jtreg/compiler/c2/cr6340864/TestLongVectRotate.java

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 3, 2021

@jatin-bhateja 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:

8271589: fatal error with variable shift count integer rotate operation.

Reviewed-by: kvn, sviswanathan

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 44 new commits pushed to the master branch:

  • 452f7d7: 8271217: Fix race between G1PeriodicGCTask checks and GC request
  • 221e4b9: 8270797: ShortECDSA.java test is not complete
  • 0a27f26: 8265057: G1: Investigate removal of maintenance of two BOT thresholds
  • eec64f5: 8256844: Make NMT late-initializable
  • 4df1bc4: 6350025: API documentation for JOptionPane using deprecated methods.
  • efcdcc7: 8270893: IndexOutOfBoundsException while reading large TIFF file
  • 977b8c4: 8271836: runtime/ErrorHandling/ClassPathEnvVar.java fails with release VMs
  • 04134fc: 8264543: Cross modify fence optimization for x86
  • 9e76909: 8271824: mark hotspot runtime/CompressedOops tests which ignore external VM flags
  • e49b7d9: 8271828: mark hotspot runtime/classFileParserBug tests which ignore external VM flags
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/7cc1eb3e571e00f1cbfd62eb843df96ba8e88199...master

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 master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Aug 3, 2021
}
shiftRCnt = cnt;
Copy link

@sviswa7 sviswa7 Aug 3, 2021

Choose a reason for hiding this comment

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

This seems redundant. shiftRCnt is being overwritten in the very next statement.

Loading

Copy link

@sviswa7 sviswa7 Aug 3, 2021

Choose a reason for hiding this comment

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

Other than this redundant statement, the patch looks good to me.

Loading

sviswa7
sviswa7 approved these changes Aug 4, 2021
@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Aug 4, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 4, 2021

Going to push as commit 392fcc9.
Since your change was applied there have been 45 commits pushed to the master branch:

  • 9f1edaf: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places
  • 452f7d7: 8271217: Fix race between G1PeriodicGCTask checks and GC request
  • 221e4b9: 8270797: ShortECDSA.java test is not complete
  • 0a27f26: 8265057: G1: Investigate removal of maintenance of two BOT thresholds
  • eec64f5: 8256844: Make NMT late-initializable
  • 4df1bc4: 6350025: API documentation for JOptionPane using deprecated methods.
  • efcdcc7: 8270893: IndexOutOfBoundsException while reading large TIFF file
  • 977b8c4: 8271836: runtime/ErrorHandling/ClassPathEnvVar.java fails with release VMs
  • 04134fc: 8264543: Cross modify fence optimization for x86
  • 9e76909: 8271824: mark hotspot runtime/CompressedOops tests which ignore external VM flags
  • ... and 35 more: https://git.openjdk.java.net/jdk/compare/7cc1eb3e571e00f1cbfd62eb843df96ba8e88199...master

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Aug 4, 2021

@jatin-bhateja Pushed as commit 392fcc9.

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

Loading

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