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

8278171: [vectorapi] Mask incorrectly computed for zero extending cast #6634

Closed

Conversation

merykitty
Copy link
Contributor

@merykitty merykitty commented Dec 1, 2021

When doing an unsigned upcast, we perform a signed cast followed by a bitwise and operation to clip the extended signed bit. The sign clip mask is currently calculated incorrectly, the correct mask should have the number of least significant bit set equal to the size of the source elements. This patch fixes this trivial issue and adds several tests for zero extension casts.

Thank you very much.


Progress

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

Issue

  • JDK-8278171: [vectorapi] Mask incorrectly computed for zero extending cast

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6634

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 1, 2021

👋 Welcome back merykitty! 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.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

@merykitty The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot core-libs labels Dec 1, 2021
@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 1, 2021

/label add hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler label Dec 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

@merykitty
The hotspot-compiler label was successfully added.

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 2, 2021

@PaulSandoz Could you take a look at this PR? Also, could you create an issue for this PR, please.
Should this be split into 2, the first one fixes the bug and add tests while the second one implements the intrinsics.
Thank you very much.

Copy link
Member

@PaulSandoz PaulSandoz left a comment

I am inclined to separated out. Fix the bug, add the tests, and integrate for 18. Then enhance with the intrinsics for 19.

If you agree to that I will create two bugs.

case Op_VectorUCastB2X:
if (size_in_bits == 256 && UseAVX < 2) {
return false; // Implementation limitation
}
break;
case Op_VectorUCastS2X:
if (size_in_bits == 256 && UseAVX < 2) {
return false;
}
break;
case Op_VectorUCastI2X:
if (size_in_bits == 256 && UseAVX < 2) {
return false;
}
break;
Copy link
Member

@PaulSandoz PaulSandoz Dec 2, 2021

Choose a reason for hiding this comment

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

Collapse cases, since each has the code code?

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 2, 2021

@PaulSandoz Yes, I think that should be the case, thank you very much.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Dec 2, 2021

@merykitty here you go:

[vectorapi] Mask incorrectly computed for zero extending cast
https://bugs.openjdk.java.net/browse/JDK-8278171

[vectorapi] Add x64 intrinsics for unsigned (zero extended) casts
https://bugs.openjdk.java.net/browse/JDK-8278173

@merykitty merykitty changed the title x86 - Vector unsigned casts intrinsics 8278171: [vectorapi] Mask incorrectly computed for zero extending cast Dec 2, 2021
@openjdk openjdk bot added the rfr label Dec 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2021

Webrevs

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 2, 2021

/label remove hotspot,hotspot-compiler

@openjdk openjdk bot removed hotspot hotspot-compiler labels Dec 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2021

@merykitty
The hotspot label was successfully removed.

The hotspot-compiler label was successfully removed.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Dec 2, 2021

Looks good. Running tests.

Something we should consider later: split this test out into smaller files and support the max vector. On supported platforms we could consider using the IR test framework to verify the correct IR node is generated.

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Tests are good.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

⚠️ @merykitty the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout vectorUnsignedCastIntrinsics
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

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

8278171: [vectorapi] Mask incorrectly computed for zero extending cast

Reviewed-by: psandoz

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

  • 19ce33d: 8258512: serviceability/sa/TestJmapCore.java timed out on macOS 10.13.6
  • c93552c: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
  • b8ac0d2: 8276681: Additional malformed Javadoc inline tags in JDK source
  • b310f30: 8277396: [TESTBUG] In DefaultButtonModelCrashTest.java, frame is accessed from main thread
  • 15345e3: 8276661: (fs) UserDefinedFileAttributeView no longer works with long path (win)
  • 652b5f8: 8276674: Malformed Javadoc inline tags in JDK source
  • e0f1fc7: 8277358: Accelerate CRC32-C
  • 73a9654: 8276806: Use Objects.checkFromIndexSize where possible in java.base
  • 30087cc: 8275342: Change nested classes in java.prefs to static nested classes
  • 669c90e: 8278099: two sun/security/pkcs11/Signature tests failed with AssertionError
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/7c4ef3abaff55b29dddc50f2aea8a04a3214f9fb...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.

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 (@PaulSandoz) 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 Dec 3, 2021
@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 3, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

@merykitty
Your change (at version 4e91375) is now ready to be sponsored by a Committer.

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Dec 3, 2021

Thanks a lot for your support and review. Do I need another review here?

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Dec 3, 2021

/sponsor

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Dec 3, 2021

Thanks a lot for your support and review. Do I need another review here?

No need for this one, for next PR for the intrinsics will likely need two HotSpot reviewers.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

Going to push as commit 2e30fa9.
Since your change was applied there have been 32 commits pushed to the master branch:

  • fbf096e: 8251400: Fix incorrect addition of library to test in JDK-8237858
  • 0a09092: 8268288: jdk/jfr/api/consumer/streaming/TestOutOfProcessMigration.java fails with "Error: ShouldNotReachHere()"
  • 0d938ce: 8278172: java/nio/channels/FileChannel/BlockDeviceSize.java should only run on Linux
  • 0e7b6bc: 8278141: LIR_OpLoadKlass::_info shadows the field of the same name from LIR_Op
  • 53a4342: 8278137: JFR: PrettyWriter uses incorrect year specifier
  • f723779: 8278079: C2: expand_dtrace_alloc_probe doesn't take effect in macro.cpp
  • 3f28a21: 8278144: Javadoc for MemorySegment::set/MemorySegment::setAtIndex is missing throws tag
  • ba2a8e5: 8278205: jlink plugins should dump .class file in debug mode
  • 18c54b4: 8274640: Cleanup unnecessary null comparison before instanceof check in java.desktop
  • dda8f26: 8278139: G1: Refactor G1BlockOffsetTablePart::block_at_or_preceding
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/7c4ef3abaff55b29dddc50f2aea8a04a3214f9fb...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Dec 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

@PaulSandoz @merykitty Pushed as commit 2e30fa9.

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

@merykitty merykitty deleted the vectorUnsignedCastIntrinsics branch Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
2 participants