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

8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if #17394

Closed
wants to merge 1 commit into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Jan 12, 2024

The assertion added by JDK-8299259 is wrong. I've originally assumed that at this point, there are no pinned Div/Mod nodes anymore that we possibly want to split through a phi due to bailing out earlier here:

// Determine if the Node has inputs from some local Phi.
// Returns the block to clone thru.
Node *n_blk = has_local_phi_input( n );
if( !n_blk ) return n;

The assumption was that a Div/Mod node with a control input to the zero-check IfProj could only have a Phi input with a Region that is further up in the graph. This is normally true. However, in testIntDiv(), we split an If with do_split_if() and need to empty the basic block. We split the store iFld = sub up. This includes the StoreI as well as the AddI which also has the Region as current ctrl (i.e. returned with get_ctrl()).

The AddI also has the DivI as output which, however, is not pushed up since it's not part of the same basic block. We end up with the following graph after the completion of do_split_if():

image

The DivI now has 252 Phi as input which merges the split AddI nodes. 246 Region of the 252 Phi is further down than the control input 83 IfTrue of the DivI.This is rather unusual and thus was missed when the assert was added in JDK-8299259. When finally processing the DivI node in the DFS walk of Split-If, we fail with the assertion.

The fix is straight forward to turn this assert into a simple bailout: We should not split a Div/Mod node that is pinned (i.e. has a zero check).

While working on this bug, I've also tried to trigger the assert with DivL/ModL nodes. However, this did not work because split_up() does not split the Add node up. The reason is that we set late ctrl to early ctrl for the DivL/ModL node (and thus also set the same late ctrl for the Add node) while we do not do that for DivI/ModI nodes. It seems that we miss to treat DivL/ModL nodes as unpinned here which would allow us to set a later ctrl:

// We'd like +VerifyLoopOptimizations to not believe that Mod's/Loads
// _must_ be pinned (they have to observe their control edge of course).
// Unlike Stores (which modify an unallocable resource, the memory
// state), Mods/Loads can float around. So free them up.
switch( n->Opcode() ) {
case Op_DivI:
case Op_DivF:
case Op_DivD:
case Op_ModI:
case Op_ModF:
case Op_ModD:

I've done some digging and found that DivL/ModL nodes were added after this switch statment. So, I assume we simply forgot to also treat them as unpinned here. It's not wrong but I think just an unnecessary limitation. I filed JDK-8323652 to fix this.

Either way, I left the code for the long cases in even though they do not trigger. They should once JDK-8323652 is fixed.

Thanks,
Christian


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if (Bug - P2)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17394/head:pull/17394
$ git checkout pull/17394

Update a local copy of the PR:
$ git checkout pull/17394
$ git pull https://git.openjdk.org/jdk.git pull/17394/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17394

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17394.diff

Webrev

Link to Webrev Comment

…check should already have bailed out earlier in split-if
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 12, 2024

👋 Welcome back chagedorn! 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 rfr Pull request is ready for review label Jan 12, 2024
@openjdk
Copy link

openjdk bot commented Jan 12, 2024

@chhagedorn The following label will be automatically applied to this pull request:

  • hotspot-compiler

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 pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 12, 2024
@mlbridge
Copy link

mlbridge bot commented Jan 12, 2024

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

Good.

@openjdk
Copy link

openjdk bot commented Jan 12, 2024

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

8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if

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

  • 62fd26f: 8323700: Add fontconfig requirement to building.md for Alpine Linux
  • 8c238ed: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature
  • cd0fe37: 8323641: Test compiler/loopopts/superword/TestAlignVectorFuzzer.java timed out
  • 45c65e6: 8323577: C2 SuperWord: remove AlignVector restrictions on IR tests added in JDK-8305055
  • 8643cc2: 8323610: G1: HeapRegion pin count should be size_t to avoid overflows
  • e66a76f: 8323660: Serial: Fix header ordering and indentation
  • ba3c3bb: 8323519: Add applications/ctw/modules to Hotspot tiered testing
  • 922f8e4: 8323693: Update some copyright announcements in the new files created in 8234502
  • 1515bd7: 8322077: Add Ideal transformation: (~a) | (~b) => ~(a & b)
  • bdee968: 4760025: sRGB conversions to and from CIE XYZ incorrect
  • ... and 168 more: https://git.openjdk.org/jdk/compare/9ab29f8dcd1c0092e4251f996bd53c704e87a74a...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 Jan 12, 2024
@chhagedorn
Copy link
Member Author

Thanks Vladimir for your review!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@chhagedorn
Copy link
Member Author

Thanks Tobias for your review!

@chhagedorn
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Jan 15, 2024

Going to push as commit 7e0a4ed.
Since your change was applied there have been 179 commits pushed to the master branch:

  • 34f85ee: 8323584: AArch64: Unnecessary ResourceMark in NativeCall::set_destination_mt_safe
  • 62fd26f: 8323700: Add fontconfig requirement to building.md for Alpine Linux
  • 8c238ed: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature
  • cd0fe37: 8323641: Test compiler/loopopts/superword/TestAlignVectorFuzzer.java timed out
  • 45c65e6: 8323577: C2 SuperWord: remove AlignVector restrictions on IR tests added in JDK-8305055
  • 8643cc2: 8323610: G1: HeapRegion pin count should be size_t to avoid overflows
  • e66a76f: 8323660: Serial: Fix header ordering and indentation
  • ba3c3bb: 8323519: Add applications/ctw/modules to Hotspot tiered testing
  • 922f8e4: 8323693: Update some copyright announcements in the new files created in 8234502
  • 1515bd7: 8322077: Add Ideal transformation: (~a) | (~b) => ~(a & b)
  • ... and 169 more: https://git.openjdk.org/jdk/compare/9ab29f8dcd1c0092e4251f996bd53c704e87a74a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 15, 2024
@openjdk openjdk bot closed this Jan 15, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 15, 2024
@openjdk
Copy link

openjdk bot commented Jan 15, 2024

@chhagedorn Pushed as commit 7e0a4ed.

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

@chhagedorn
Copy link
Member Author

/backport jdk22

@openjdk
Copy link

openjdk bot commented Jan 15, 2024

@chhagedorn the backport was successfully created on the branch backport-chhagedorn-7e0a4ed6 in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7e0a4ed6 from the openjdk/jdk repository.

The commit being backported was authored by Christian Hagedorn on 15 Jan 2024 and was reviewed by Vladimir Kozlov and Tobias Hartmann.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22:

$ git fetch https://github.com/openjdk-bots/jdk22.git backport-chhagedorn-7e0a4ed6:backport-chhagedorn-7e0a4ed6
$ git checkout backport-chhagedorn-7e0a4ed6
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22.git backport-chhagedorn-7e0a4ed6

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