8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic) #31

rgiulietti · 2020-09-06T15:25:10Z

This is a follow-up of the Mercurial-based workflow initiated on the core-lib-devs mailing list [0]. Not sure if this one is strictly necessary or if the patches sent on the list are sufficient. Anyway, I exploit this PR as a test ;-)

[0] https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068474.html

Progress

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

Issue

JDK-8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic)

Reviewers

Brian Burkhalter (@bplb - Reviewer)

Download

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

…eds to be Intrinsic)

bridgekeeper · 2020-09-06T15:26:58Z

Hi @rgiulietti, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rgiulietti" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2020-09-06T15:27:44Z

@rgiulietti The following label will be automatically applied to this pull request: core-libs.

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

rgiulietti · 2020-09-06T15:51:04Z

/signed

bridgekeeper · 2020-09-06T15:52:48Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

mlbridge · 2020-09-07T17:17:13Z

Webrevs

eamonnmcmanus · 2020-09-08T20:21:59Z

src/java.base/share/classes/java/lang/Long.java

+        if (divisor >= 0) {
+            final long q = (dividend >>> 1) / divisor << 1;
+            final long r = dividend - q * divisor;
+            return q + ((r | ~(r - divisor)) >>> Long.SIZE - 1);


I think it would be worth parenthesizing the right hand side of the >>> operator here too.

eamonnmcmanus · 2020-09-08T20:22:22Z

src/java.base/share/classes/java/lang/Long.java

        }
+        return (dividend & ~(dividend - divisor)) >>> Long.SIZE - 1;


Parenthesize RHS?

eamonnmcmanus · 2020-09-08T20:30:36Z

src/java.base/share/classes/java/lang/Long.java

+        if (divisor >= 0) {
+            final long q = (dividend >>> 1) / divisor << 1;
+            final long r = dividend - q * divisor;
+            return r - (~(r - divisor) >> Long.SIZE - 1 & divisor);


Parentheses would be particularly helpful here. I'd certainly have to think hard about the relative precedences of >>, -, and &, whereas I wouldn't have to with:

return r - ((~(r - divisor) >> (Long.SIZE - 1)) & divisor);

I also think it would be worth adding a comment saying that this is deliberately >> not >>>, even though we have >>> in the divide method. Here we're propagating the sign bit so that thing & divisor is either 0 or divisor according as thing is –1 or 0.

Hi Éamonn,

I have no problems adding non-strictly needed parentheses. I usually don't because I'm quite familiar with operator precedence and often forget that other people are not.

I'll add comments for parts, like the one you point out, that are not already discussed at length in the bibliographical reference.

Thanks
Raffaello

Hi,

I just committed the changes.

Greetings
Raffaello

…eds to be Intrinsic) Added helpful parentheses and comments to clarify aspects not discussed in "Hacker's Delight".

shipilev

Since this patch does not involve adding the intrinsic, maybe drop "(Needs to be intrinsic)" from the synopsis?

rgiulietti · 2020-09-09T07:38:38Z

@shipilev I agree, but that's the title chosen by the reporter of the JBS issue. Not sure I can edit it without impacting bots and tools.

rgiulietti · 2020-09-24T15:33:01Z

Hello

this is a gentle reminder that this issue still needs a more formal review.

Greetings
Raffaello

rgiulietti · 2020-10-14T14:20:11Z

Hello,

a reminder that this issue has not been formally reviewed.
Estimated time: 10-15 min with "Hacker's delight" on the desk.

Greetings
Raffaello

bplb · 2020-10-17T00:16:57Z

I think the change looks good.

Before the project was on GitHub, there was a change to the test posted in http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068504.html to address the comment in http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068484.html. I think the test is all right but perhaps someone else still has a comment. The copyright year in any case needs to be adjusted.

I can sponsor this change if you like.

rgiulietti · 2020-10-19T15:18:58Z

@bpb Since the switch to Skara, the test is on GitHub as well.
I just modified the copyright year some minutes ago.

bplb · 2020-10-19T17:03:00Z

OK, looks good. Let's leave the PR open for a day or two and then move forward. Thanks.

bplb

Seeing no more comments in the last couple of days, I think this can be integrated.

openjdk · 2020-10-21T16:17:34Z

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

8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic)

Reviewed-by: bpb

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

365f19c: 8254790: SIGSEGV in string_indexof_char and stringL_indexof_char intrinsics
f813a28: 8254692: (se) Clarify the behaviour of the non-abstract SelectorProvider::inheritedChannel
c9269bf: 8255036: Shenandoah: Reset GC state for root verifier
839f01d: 8242068: Signed JAR support for RSASSA-PSS and EdDSA
e559bd2: 8254889: name_and_sig_as_C_string usages in frame coding without ResourceMark
da97ab5: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer
7e26404: 8255000: C2: Unify IGVN processing when loop opts are over
27230fa: 8255026: C2: Miscellaneous cleanups in Compile and PhaseIdealLoop code
c107178: 8253964: [Graal] UnschedulableGraphTest#test01fails with expected:<4> but was:<3>
bd45191: 8255065: Zero: accessor_entry misses the IRIW case
... and 612 more: https://git.openjdk.java.net/jdk/compare/4b3a0b789ebf389a9589c9af2d3922fb91916d92...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 (@shipilev, @bplb) 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).

rgiulietti · 2020-10-21T16:26:36Z

/integrate

openjdk · 2020-10-21T16:27:57Z

@rgiulietti
Your change (at version efb8c71) is now ready to be sponsored by a Committer.

bplb · 2020-10-21T16:31:35Z

/sponsor

openjdk · 2020-10-21T16:33:33Z

@bplb @rgiulietti Since your change was applied there have been 622 commits pushed to the master branch:

365f19c: 8254790: SIGSEGV in string_indexof_char and stringL_indexof_char intrinsics
f813a28: 8254692: (se) Clarify the behaviour of the non-abstract SelectorProvider::inheritedChannel
c9269bf: 8255036: Shenandoah: Reset GC state for root verifier
839f01d: 8242068: Signed JAR support for RSASSA-PSS and EdDSA
e559bd2: 8254889: name_and_sig_as_C_string usages in frame coding without ResourceMark
da97ab5: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer
7e26404: 8255000: C2: Unify IGVN processing when loop opts are over
27230fa: 8255026: C2: Miscellaneous cleanups in Compile and PhaseIdealLoop code
c107178: 8253964: [Graal] UnschedulableGraphTest#test01fails with expected:<4> but was:<3>
bd45191: 8255065: Zero: accessor_entry misses the IRIW case
... and 612 more: https://git.openjdk.java.net/jdk/compare/4b3a0b789ebf389a9589c9af2d3922fb91916d92...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0efdde1.

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

In AArch64, asr x10, x1, openjdk#31 neg x0, x10 can be optimized to: neg x0, x1, asr openjdk#31 To implement the instruction combining, we add matching rules in the backend. Change-Id: Iaee06f7a03e97a7e092e13da75812f3722549c3b

JVM-1693: Materialization updates AddP which is not dominated by current control

They are only timeouting on CI because they take time and machines on GHA are slow. We need a better solution than disabling them. (retries?)

8238669: Long.divideUnsigned is extremely slow for certain values (Ne…

6ae462c

…eds to be Intrinsic)

bridgekeeper bot added the oca Needs verification of OCA signatory status label Sep 6, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 6, 2020

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Sep 6, 2020

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Sep 7, 2020

openjdk bot added the rfr Pull request is ready for review label Sep 7, 2020

eamonnmcmanus reviewed Sep 8, 2020

View reviewed changes

8238669: Long.divideUnsigned is extremely slow for certain values (Ne…

e156372

…eds to be Intrinsic) Added helpful parentheses and comments to clarify aspects not discussed in "Hacker's Delight".

shipilev reviewed Sep 9, 2020

View reviewed changes

Modified copyright year.

efb8c71

bplb approved these changes Oct 21, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 21, 2020

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 21, 2020

openjdk bot closed this Oct 21, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 21, 2020

rgiulietti deleted the JDK-8238669 branch October 21, 2020 20:56

mlbridge bot mentioned this pull request May 25, 2021

8262891: Compiler implementation for Pattern Matching for switch (Preview) #3863

Closed

3 tasks

caojoshua pushed a commit to caojoshua/jdk that referenced this pull request May 8, 2023

Merge pull request openjdk#31 from navyxliu/JVM-1963

85cb112

JVM-1693: Materialization updates AddP which is not dominated by current control

robehn pushed a commit to robehn/jdk that referenced this pull request Aug 15, 2023

Enable tests passing locally on riscv64 (openjdk#31)

fd6b110

They are only timeouting on CI because they take time and machines on GHA are slow. We need a better solution than disabling them. (retries?)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic) #31

8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic) #31

rgiulietti commented Sep 6, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 6, 2020

openjdk bot commented Sep 6, 2020 •

edited by bridgekeeper bot

Loading

rgiulietti commented Sep 6, 2020

bridgekeeper bot commented Sep 6, 2020

mlbridge bot commented Sep 7, 2020 •

edited by bridgekeeper bot

Loading

eamonnmcmanus Sep 8, 2020 •

edited

Loading

eamonnmcmanus Sep 8, 2020

eamonnmcmanus Sep 8, 2020

rgiulietti Sep 8, 2020

rgiulietti Sep 8, 2020

shipilev left a comment

rgiulietti commented Sep 9, 2020

rgiulietti commented Sep 24, 2020

rgiulietti commented Oct 14, 2020

bplb commented Oct 17, 2020 •

edited

Loading

rgiulietti commented Oct 19, 2020

bplb commented Oct 19, 2020

bplb left a comment

openjdk bot commented Oct 21, 2020 •

edited

Loading

rgiulietti commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

bplb commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

		}
		return (dividend & ~(dividend - divisor)) >>> Long.SIZE - 1;

8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic) #31

8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic) #31

Conversation

rgiulietti commented Sep 6, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 6, 2020

openjdk bot commented Sep 6, 2020 • edited by bridgekeeper bot Loading

rgiulietti commented Sep 6, 2020

bridgekeeper bot commented Sep 6, 2020

mlbridge bot commented Sep 7, 2020 • edited by bridgekeeper bot Loading

Webrevs

eamonnmcmanus Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

eamonnmcmanus Sep 8, 2020

Choose a reason for hiding this comment

eamonnmcmanus Sep 8, 2020

Choose a reason for hiding this comment

rgiulietti Sep 8, 2020

Choose a reason for hiding this comment

rgiulietti Sep 8, 2020

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

rgiulietti commented Sep 9, 2020

rgiulietti commented Sep 24, 2020

rgiulietti commented Oct 14, 2020

bplb commented Oct 17, 2020 • edited Loading

rgiulietti commented Oct 19, 2020

bplb commented Oct 19, 2020

bplb left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 21, 2020 • edited Loading

rgiulietti commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

bplb commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

rgiulietti commented Sep 6, 2020 •

edited by openjdk bot

Loading

openjdk bot commented Sep 6, 2020 •

edited by bridgekeeper bot

Loading

mlbridge bot commented Sep 7, 2020 •

edited by bridgekeeper bot

Loading

eamonnmcmanus Sep 8, 2020 •

edited

Loading

bplb commented Oct 17, 2020 •

edited

Loading

openjdk bot commented Oct 21, 2020 •

edited

Loading