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

8255150: Add utility methods to check long indexes and ranges #1003

Closed
wants to merge 27 commits into from

Conversation

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Nov 2, 2020

This change add 3 new methods in Objects:

public static long checkIndex(long index, long length)
public static long checkFromToIndex(long fromIndex, long toIndex, long length)
public static long checkFromIndexSize(long fromIndex, long size, long length)

This mirrors the int utility methods that were added by JDK-8135248
with the same motivations.

As is the case with the int checkIndex(), the long checkIndex() method
is JIT compiled as an intrinsic. It allows the JIT to compile
checkIndex to an unsigned comparison and properly recognize it as
a range check that then becomes a candidate for the existing range check
optimizations. This has proven to be important for panama's
MemorySegment API and a prototype of this change (with some extra c2
improvements) showed that panama micro benchmark results improve
significantly.

This change includes:

  • the API change
  • the C2 intrinsic
  • tests for the API and the C2 intrinsic

This is a joint work with Paul who reviewed and reworked the API change
and filled the CSR.

/cc core-libs,hotspot-compiler


Progress

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

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (6/6 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8255150: Add utility methods to check long indexes and ranges

Reviewers

Contributors

  • Paul Sandoz <psandoz@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1003/head:pull/1003
$ git checkout pull/1003

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 2, 2020

👋 Welcome back roland! 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 openjdk bot added the core-libs label Nov 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2020

@rwestrel
The core-libs label was successfully added.

The hotspot-compiler label was successfully added.

@rwestrel rwestrel marked this pull request as ready for review Nov 2, 2020
@openjdk openjdk bot added the rfr label Nov 2, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2020

Copy link
Member

@JornVernee JornVernee left a comment

Hi Roland, Paul. Thank you for tackling this! We have to jump through quite a few hoops in the implementation of the foreign memory access API in order to leverage the intrinsification of int-based index checks, and even then we are not covering the cases where the numbers are larger than ints. Looking forward to being able to remove those hacks!

Although I'm not a 'Reviewer', I've done a pass over the code and left some inline comments that I hope you find useful.

assert((jlong)(jint)con == con, "not an int");
return intcon((jint) con);
Comment on lines 108 to 109

This comment has been minimized.

@JornVernee

JornVernee Nov 5, 2020
Member

Could also use checked_cast here (from globalDefinitions.hpp), which does a similar check.

Suggested change
assert((jlong)(jint)con == con, "not an int");
return intcon((jint) con);
return intcon(checked_cast<jint>(con));
src/hotspot/share/opto/graphKit.hpp Show resolved Hide resolved
jint int_lo = (jint)lo;
jint int_hi = (jint)hi;
assert(((jlong)int_lo) == lo && ((jlong)int_hi) == hi, "bounds are not ints");
return TypeInt::make(int_lo, int_hi, w);
Comment on lines 1350 to 1353

This comment has been minimized.

@JornVernee

JornVernee Nov 5, 2020
Member

checked_cast could also be used here.

Suggested change
jint int_lo = (jint)lo;
jint int_hi = (jint)hi;
assert(((jlong)int_lo) == lo && ((jlong)int_hi) == hi, "bounds are not ints");
return TypeInt::make(int_lo, int_hi, w);
return TypeInt::make(checked_cast<jint>(lo), checked_cast<jint>(hi), w);
interface X {
long apply(long a, long b, long c);
}

Comment on lines 91 to 94

This comment has been minimized.

@JornVernee

JornVernee Nov 5, 2020
Member

Seems unused?

Suggested change
interface X {
long apply(long a, long b, long c);
}
@rwestrel rwestrel force-pushed the rwestrel:JDK-8255150 branch from 74df12d to b47184a Nov 5, 2020
@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 5, 2020

Although I'm not a 'Reviewer', I've done a pass over the code and left some inline comments that I hope you find useful.

Thanks for the review! All suggestions (except the exception message one that I commented on) look good to me. I applied them.

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented Nov 5, 2020

/csr

@openjdk openjdk bot added the csr label Nov 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2020

@ChrisHegarty this pull request will not be integrated until the CSR request JDK-8255151 for issue JDK-8255150 has been approved.

Copy link
Member

@dean-long dean-long left a comment

C2 changes look good, except for new code block in inline_preconditions_checkIndex could use a comment.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 6, 2020

C2 changes look good, except for new code block in inline_preconditions_checkIndex could use a comment.

Thanks for the review. I added a comment for this block and some other comments for the rest of this method.

@@ -96,6 +96,12 @@ Node* ConstraintCastNode::make_cast(int opcode, Node* c, Node *n, const Type *t,
cast->set_req(0, c);
return cast;
}
case Op_CastLL: {
assert(!carry_dependency, "carry dependency not supported");

This comment has been minimized.

@iwanowww

iwanowww Nov 7, 2020

Any particular reason to reject control dependency (except it is not used right now)?

This comment has been minimized.

@rwestrel

rwestrel Nov 9, 2020
Author Contributor

Because it's not used. But actually now that I think more about it, I realize it's required. I updated the change with support for carry_dependency for CastLL.
Thanks for the review!

@rwestrel rwestrel force-pushed the rwestrel:JDK-8255150 branch from aaacd32 to 692b429 Nov 9, 2020
@openjdk openjdk bot removed the csr label Nov 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2020

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

8255150: Add utility methods to check long indexes and ranges

Co-authored-by: Paul Sandoz <psandoz@openjdk.org>
Reviewed-by: jvernee, dlong, vlivanov

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 master branch:

  • 6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
  • 5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 13, 2020

@rwestrel this pull request can not be integrated into master 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-8255150
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@openjdk
Copy link

@openjdk openjdk bot commented Nov 13, 2020

⚠️ @rwestrel This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk openjdk bot added merge-conflict and removed ready labels Nov 13, 2020
@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 13, 2020

I noticed the compiler test would fail with -Xcomp. I pushed a fix for that

@openjdk openjdk bot added ready and removed merge-conflict labels Nov 13, 2020
@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 17, 2020

/reviewer credit jvernee

@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@rwestrel Reviewer jvernee has already made an authenticated review of this PR, and does not need to be credited manually.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 17, 2020

/reviewer credit dlong

@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@rwestrel Reviewer dlong has already made an authenticated review of this PR, and does not need to be credited manually.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 17, 2020

/contributor add psandoz

@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@rwestrel
Contributor Paul Sandoz <psandoz@openjdk.org> successfully added.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Nov 17, 2020

/integrate

@openjdk openjdk bot closed this Nov 17, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

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

  • 6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
  • 5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check

Your commit was automatically rebased without conflicts.

Pushed as commit a7422ac.

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

openjdk-notifier bot referenced this pull request Nov 17, 2020
Co-authored-by: Paul Sandoz <psandoz@openjdk.org>
Reviewed-by: jvernee, dlong, vlivanov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants