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

8253923: C2 doesn't always run loop opts for compilations that include loops #478

Closed
wants to merge 1 commit into from

Conversation

rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Oct 2, 2020

/cc hotspot-compiler


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) (1/9 failed) ✔️ (9/9 passed)

Failed test task

Issue

  • JDK-8253923: C2 doesn't always run loop opts for compilations that include loops

Reviewers

Download

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

@rwestrel rwestrel marked this pull request as draft Oct 2, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 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 hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 2, 2020
@openjdk
Copy link

openjdk bot commented Oct 2, 2020

@rwestrel
The hotspot-compiler label was successfully added.

@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 2, 2020

I noticed that c2 wouldn't always run loop opts when the compilation include loops. Compile::has_loops() is what controls whether loop opts are executed. It's initially false and then set to true as parsing finds new loops. The flag is updated with the result ciMethod::has_loops() of inlined methods but:

  1. with bimorphic inlining, the has_loops flag is not set based on the methods that are actually inlined but from the virtual method the call resolve statically to.

  2. there's a least one intrinsic that's predicated and falls back to inlining of the method bytecodes, in which case has_loops is not set.

  3. sometimes loops are built manually rather than by parsing bytecodes

  4. when the backedge is a branch of a switch or an exception edge, ciMethod::has_loops() doesn't return true

I propose that 1 & 2 be fixed by setting the has_loops flag when parsing of a method starts rather than when a call is inlined. 3 is fixed by explicitly setting the flag where the loop is built, 4 by extending ciMethod::has_loops() for switches.

Copy link

@neliasso neliasso left a comment

Thanks for fixing!

Extra credit for the verification that will catch this problem type in the future.

Reviewed.

@openjdk
Copy link

openjdk bot commented Oct 4, 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:

8253923: C2 doesn't always run loop opts for compilations that include loops

Reviewed-by: neliasso, 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 190 new commits pushed to the master branch:

  • 295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
  • 0fab73e: 8254560: Shenandoah: Concurrent Strong Roots logging is incorrect
  • 638f910: 8254559: Remove unimplemented JVMFlag::get_locked_message_ext
  • 0ec1d63: 8253117: Replace HTML tables in javadoc summaries with CSS grid elements
  • 54bbe76: 8251544: CTW: C2 fails with assert(no_dead_loop) failed: dead loop detected
  • 13fe054: 8253588: C1: assert(false) failed: unknown register on x86_32 only with -XX:+TraceLinearScanLevel=4
  • 59378a1: 8254164: G1 only removes self forwarding pointers for last collection set increment
  • bf46acf: 8254028: G1 incorrectly updates scan_top for collection set regions during preparation of evacuation
  • a2bb4c6: 8254314: Shenandoah: null checks in c2 should not skip over native load barrier
  • c73a0ff: 8252105: Parallel heap inspection for ZCollectedHeap
  • ... and 180 more: https://git.openjdk.java.net/jdk/compare/9c17a35e50e86eaa1274389f8a4709da3a9c6402...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 4, 2020
@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 5, 2020

Hi Nils,

Reviewed.

Thanks for reviewing. I actually made this a draft PR because I realized when I created it that there was a simpler fix (which is to set the has_loops flag when parsing of a method starts). Can you take a look at the new change?

@rwestrel rwestrel marked this pull request as ready for review Oct 5, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 5, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 5, 2020

Webrevs

@neliasso
Copy link

neliasso commented Oct 6, 2020

Thanks for reviewing. I actually made this a draft PR because I realized when I created it that there was a simpler fix (which is to set the has_loops flag when parsing of a method starts). Can you take a look at the new change?

Where do I find the new change? I see no updated files.

@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 7, 2020

Thanks for reviewing. I actually made this a draft PR because I realized when I created it that there was a simpler fix (which is to set the has_loops flag when parsing of a method starts). Can you take a look at the new change?

Where do I find the new change? I see no updated files.

I updated the change in place (force pushed).
https://openjdk.github.io/cr/?repo=jdk&pr=478&range=00 is the new change.

@neliasso
Copy link

neliasso commented Oct 8, 2020

I updated the change in place (force pushed).
https://openjdk.github.io/cr/?repo=jdk&pr=478&range=00 is the new change.

That looks exactly like the code I reviewed.

@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 8, 2020

I updated the change in place (force pushed).
https://openjdk.github.io/cr/?repo=jdk&pr=478&range=00 is the new change.

That looks exactly like the code I reviewed.

The initial change would set the has_loops flag before inlining. The current one sets it when a new method is parsed (so once inlining has happened). In any case, I take it you're fine with the current change. Thanks for the review.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 12, 2020

Looks good to me.

Thanks for the review.

@rwestrel
Copy link
Contributor Author

rwestrel commented Oct 12, 2020

/integrate

@openjdk openjdk bot closed this Oct 12, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 12, 2020
@openjdk
Copy link

openjdk bot commented Oct 12, 2020

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

  • dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
  • 295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
  • 0fab73e: 8254560: Shenandoah: Concurrent Strong Roots logging is incorrect
  • 638f910: 8254559: Remove unimplemented JVMFlag::get_locked_message_ext
  • 0ec1d63: 8253117: Replace HTML tables in javadoc summaries with CSS grid elements
  • 54bbe76: 8251544: CTW: C2 fails with assert(no_dead_loop) failed: dead loop detected
  • 13fe054: 8253588: C1: assert(false) failed: unknown register on x86_32 only with -XX:+TraceLinearScanLevel=4
  • 59378a1: 8254164: G1 only removes self forwarding pointers for last collection set increment
  • bf46acf: 8254028: G1 incorrectly updates scan_top for collection set regions during preparation of evacuation
  • a2bb4c6: 8254314: Shenandoah: null checks in c2 should not skip over native load barrier
  • ... and 181 more: https://git.openjdk.java.net/jdk/compare/9c17a35e50e86eaa1274389f8a4709da3a9c6402...master

Your commit was automatically rebased without conflicts.

Pushed as commit a6c23b7.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
3 participants