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

8238812: assert(false) failed: bad AD file #1563

Closed
wants to merge 3 commits into from

Conversation

@r-v-raghav
Copy link
Member

@r-v-raghav r-v-raghav commented Dec 2, 2020

Request help to review this 8238812 fix.

https://bugs.openjdk.java.net/browse/JDK-8238812

Found the root cause of the problem as that the following type
two If checks are not folded into one for the sample test:

170 ConI === 0 [[ 171 ]] #int:31
171 CmpI === _ 144 170 [[ 172 179 ]]
172 Bool === _ 171 [[ 173 ]] [ne]

173 If === 158 172 [[ 176 177 ]]
176 IfFalse === 173 [[ 168 ]] #0
177 IfTrue === 173 [[ 180 ]] #1
179 Bool === _ 171 [[ 180 ]] [lt]

180 If === 177 179 [[ 181 182 ]]
181 IfTrue === 180 [[ 168 ]] #1
182 IfFalse === 180 [[ 218 ]] #0

Where 144 is the value and the ifs check for:
value != 31
value < 31

Now if we reach 182 IfFalse, the following holds:
value != 31 && !(value < 31)
=> value != 31 && value >= 31
=> value > 31

But the type of value (144) becomes #int:16..31 :
140 ConI === 0 [[ 141 ]] #int:2
135 Phi === 534 481 528 [[ ... ]] #int:1..4 #tripcount
141 LShiftI === _ 135 140 [[ 142 ]] #int:4..16
143 ConI === 0 [[ 144 ]] #int:11
142 AddI === _ 141 135 [[ 144 500 485 ]] #int:5..20
144 AddI === _ 142 143 [[ ... ]] #int:16..31

And that means that value > 31 is always false and should be folded.
The root cause of the bug is that this does not happen.
Understood that the fix by Tobias for JDK-8236721 should optimize exactly above type cases
https://hg.openjdk.java.net/jdk/jdk/rev/9c53fdf6ba63

Found the IfNode::fold_compares optmization (and 8236721 related changes by Tobias)
is not called for 180-If, after the type of 144-AddI is set to #int:16..31.
because the _worklist is not correctly updated!

Proposed changes helped to add the required If node to _worklist (after the correct type setting of related value)
and with this change no failure due to existing fold_compares optimization happening later on.
Sample test passed and found no issues with various tier testing.

/reviewer credit thartmann


Progress

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

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 2, 2020

👋 Welcome back rraghavan! 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 label Dec 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@r-v-raghav The command reviewer cannot be used in the pull request body. Please use it in a new comment.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@r-v-raghav 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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2020

Webrevs

@openjdk openjdk bot added rfr and removed rfr labels Dec 3, 2020
@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Dec 3, 2020

Now added the new test case missed earlier.

@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Dec 3, 2020

/reviewer credit thartmann

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@r-v-raghav
Reviewer thartmann successfully credited.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Otherwise looks good.

@@ -1552,6 +1552,18 @@ void PhaseIterGVN::add_users_to_worklist( Node *n ) {
}
}
if (use_op == Op_CmpI) {
// Put If on worklist to enable fold_compares optimization (see IfNode::cmpi_folds)
if (use->outcnt() > 0) {
for (uint i = 0; i < use->outcnt(); i++) {
Copy link
Member

@TobiHartmann TobiHartmann Dec 3, 2020

Choose a reason for hiding this comment

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

The use->outcnt() > 0 check is redundant and you could use DUIterator_Fast instead of raw_out. Also, bol could have multiple if users. For an example, see code in countedloop_phi_from_cmp.

* @bug 8238812
* @summary Fix c2 assert(false) failed: bad AD file
* @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.Test8238812::test
* -XX:CompileCommand=dontinline,compiler.c2.Test8238812::test
Copy link
Member

@TobiHartmann TobiHartmann Dec 3, 2020

Choose a reason for hiding this comment

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

I don't think the dontinline statement is required if we are only compiling that method anyway, right?

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@r-v-raghav 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:

8238812: assert(false) failed: bad AD file

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

  • fa58671: 8257020: [JVMCI] enable a JVMCICompiler to specify which GCs it supports
  • 129c377: 8257594: C2 compiled checkcast of non-null object triggers endless deoptimization/recompilation cycle
  • e4497c9: 8256718: Obsolete the long term deprecated and aliased Trace flags
  • 4a267f1: 8244847: Linux/PPC: runtime/CompressedOops/CompressedClassPointers: smallHeapTest fails
  • b44a329: 8256864: [windows] Improve tracing for mapping errors
  • ae1eb28: 8257604: JNI_ArgumentPusherVaArg leaks valist
  • 4169d96: 8257143: Enable JVMCI code installation tests on AArch64
  • a5a034b: 8257617: TestLinkPlatform fails with new Java source version
  • d80ae05: 8166596: TLS support for the EdDSA signature algorithm
  • 3932527: 8257466: Improve enum iteration
  • ... and 48 more: https://git.openjdk.java.net/jdk/compare/b5ce8af3d7f991c0cea7b2380469fd30f3030670...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 label Dec 3, 2020
@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Dec 4, 2020

Please note suspending this PR review for now.
When tried testing the comment modifications from Tobias, started getting the same 'assert bad AD file' failure for the new testcase. On further testing got the same failure even with original fix, intermittently.
So now working to find what was missing.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 1, 2021

@r-v-raghav This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 29, 2021

@r-v-raghav This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

@bridgekeeper bridgekeeper bot closed this Jan 29, 2021
@r-v-raghav r-v-raghav deleted the JDK-8238812 branch Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants