8334650: Add debug information about whether an Assertion Predicate is for the init or last value #19856

chhagedorn · 2024-06-24T12:23:56Z

It is sometimes hard to see if an Assertion Predicate is about the init or the last value of a range check. This patch therefore adds debugging information when dumping an Assertion Predicate node to tty or for IGV:

Template Assertion Predicate:

 292  Opaque4  === _ 291 25  [[ 295 ]]  !orig=[174]
 293  IfTrue  === 295  [[ 401 ]] #1 !orig=[176]
 294  IfFalse  === 295  [[ 297 ]] #0 !orig=[177]
 295  RangeCheck  === 273 292  [[ 293 294 ]] #Init Value Assertion Predicate  P=0.999999, C=-1.000000 !orig=[175]
...
 272  Opaque4  === _ 271 25  [[ 275 ]]  !orig=[184]
 273  IfTrue  === 275  [[ 295 ]] #1 !orig=[186]
 274  IfFalse  === 275  [[ 277 ]] #0 !orig=[187]
 275  RangeCheck  === 262 272  [[ 273 274 ]] #Last Value Assertion Predicate  P=0.999999, C=-1.000000 !orig=[185]

Initialized Assertion Predicate:

 398  OpaqueInitializedAssertionPredicate  === _ 396  [[ 401 ]] 
 399  IfTrue  === 401  [[ 413 ]] #1 !orig=293,[176]
 400  IfFalse  === 401  [[ 403 ]] #0 !orig=294,[177]
 401  RangeCheck  === 293 398  [[ 399 400 ]] #Init Value Assertion Predicate  P=0.999999, C=-1.000000 !orig=295,[175]
...
 410  OpaqueInitializedAssertionPredicate  === _ 408  [[ 413 ]] 
 411  IfTrue  === 413  [[ 149 ]] #1 !orig=273,[186]
 412  IfFalse  === 413  [[ 415 ]] #0 !orig=274,[187]
 413  RangeCheck  === 399 410  [[ 411 412 ]] #Last Value Assertion Predicate  P=0.999999, C=-1.000000 !orig=275,[185]

IGV:

I could have added an additional "initialized" or "template" to better distinguish them but I eventually plan to get rid of If nodes for templates and introduce a dedicated TemplateAssertionPredicateNode. I therefore use the same strings for templates and initialized versions for now. Moreover, one can easily look at the input nodes to see if it is a template (with Opaque4) or an initialized version (with OpaqueInitializedAssertionPredicate).

Since this is only useful for debugging, I've guarded everything with NOT_PRODUCT.

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-8334650: Add debug information about whether an Assertion Predicate is for the init or last value (Sub-task - P4)

Reviewers

Roland Westrelin (@rwestrel - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19856

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

Using diff file

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

Webrev

Link to Webrev Comment

…s for the init or last value

bridgekeeper · 2024-06-24T12:24:30Z

👋 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 · 2024-06-24T12:25:36Z

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

8334650: Add debug information about whether an Assertion Predicate is for the init or last value

Reviewed-by: roland, kvn

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

6c67933: 8334899: Test runtime/cds/appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java failed after JDK-8306580
cae94b2: 8334397: RISC-V: verify perf of ReverseBytesS/US
75a2afa: 8248981: Specify list of standard message digest and mgf algorithms for RSASSA-PSS signature
baafa66: 8334287: Man page update for jstatd deprecation
c30e040: 8331911: Reconsider locking for recently disarmed nmethods
974dca8: 8334223: Make Arena MEMFLAGs immutable
e527e1c: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
3a26bbc: 8185429: [macos] After a modal dialog is closed, no window becomes active
4b153e5: 8306580: Propagate CDS dumping errors instead of directly exiting the VM
71a692a: 8321033: Avoid casting Array to GrowableArray
... and 6 more: https://git.openjdk.org/jdk/compare/ca5a438e5a4612c66f70c70a9d425eca0e49e84d...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 · 2024-06-24T12:25:59Z

@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.

chhagedorn · 2024-06-24T12:24:39Z

src/hotspot/share/opto/cfgnode.hpp

+ public:
+  float _prob;                           // Probability of true path being taken.
+  float _fcnt;                           // Frequency counter


Moved these fields up to the start of the class to have them together with the new field.

chhagedorn · 2024-06-24T12:25:31Z

src/hotspot/share/opto/cfgnode.hpp

-    init_req(1,b);
-  }
+  IfNode(Node* control, Node* bol, float p, float fcnt);
+  NOT_PRODUCT(IfNode(Node* control, Node* bol, float p, float fcnt, AssertionPredicateType assertion_predicate_type);)


New constructor to set the Assertion Predicate type explicitly. Otherwise, it is just None. Same for RangeCheck.

chhagedorn · 2024-06-24T12:26:14Z

src/hotspot/share/opto/ifnode.cpp

+  if (_useless) {
+    st->print("#useless ");
+  }


Squeezed this in here for Parse Predicates. It's probably not worth to create a separate RFE just for that.

mlbridge · 2024-06-25T06:22:23Z

Webrevs

00: Full (6c968e38)

rwestrel

Looks good to me.

chhagedorn · 2024-06-25T08:22:11Z

Thanks Roland for your review!

vnkozlov

Looks good.

chhagedorn · 2024-06-25T15:07:06Z

Thanks Vladimir for your review!

chhagedorn · 2024-06-26T07:09:11Z

/integrate

openjdk · 2024-06-26T07:09:51Z

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

25c3845: 8333133: Simplify QuickSort::sort
c66f785: 8334505: RISC-V: Several tests fail when MaxVectorSize does not match VM_Version::_initial_vector_length
f101e15: 8333583: Crypto-XDH.generateSecret regression after JDK-8329538
b3bf31a: 8333542: Breakpoint in parallel code does not work
86b0cf2: 8334653: ISO 4217 Amendment 177 Update
861aefc: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
f8bf470: 8334810: Redo: Un-ProblemList LocaleProvidersRun and CalendarDataRegression
933eaba: 8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative
7429c37: 8334598: Default classlist in JDK is not deterministic after JDK-8293980
9c89f08: 8334421: assert(!oldbox->is_unbalanced()) failed: this should not be called for unbalanced region
... and 17 more: https://git.openjdk.org/jdk/compare/ca5a438e5a4612c66f70c70a9d425eca0e49e84d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-26T07:09:57Z

@chhagedorn Pushed as commit a5f401f.

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

8334650: Add debug information about whether an Assertion Predicate i…

6c968e3

…s for the init or last value

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 24, 2024

chhagedorn commented Jun 24, 2024

View reviewed changes

chhagedorn marked this pull request as ready for review June 25, 2024 06:18

openjdk bot added the rfr Pull request is ready for review label Jun 25, 2024

rwestrel approved these changes Jun 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 25, 2024

vnkozlov approved these changes Jun 25, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 26, 2024

openjdk bot closed this Jun 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8334650: Add debug information about whether an Assertion Predicate is for the init or last value #19856

8334650: Add debug information about whether an Assertion Predicate is for the init or last value #19856

Uh oh!

chhagedorn commented Jun 24, 2024 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 24, 2024

Uh oh!

openjdk bot commented Jun 24, 2024 •

edited

Loading

Uh oh!

openjdk bot commented Jun 24, 2024

Uh oh!

chhagedorn Jun 24, 2024

Uh oh!

chhagedorn Jun 24, 2024

Uh oh!

chhagedorn Jun 24, 2024 •

edited

Loading

Uh oh!

mlbridge bot commented Jun 25, 2024

Uh oh!

rwestrel left a comment

Uh oh!

chhagedorn commented Jun 25, 2024

Uh oh!

vnkozlov left a comment

Uh oh!

chhagedorn commented Jun 25, 2024

Uh oh!

chhagedorn commented Jun 26, 2024

Uh oh!

openjdk bot commented Jun 26, 2024

Uh oh!

openjdk bot commented Jun 26, 2024

Uh oh!

Uh oh!

8334650: Add debug information about whether an Assertion Predicate is for the init or last value #19856

8334650: Add debug information about whether an Assertion Predicate is for the init or last value #19856

Uh oh!

Conversation

chhagedorn commented Jun 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 24, 2024

Uh oh!

openjdk bot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 24, 2024

Uh oh!

chhagedorn Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

chhagedorn Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

chhagedorn Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jun 25, 2024

Webrevs

Uh oh!

rwestrel left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Jun 25, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Jun 25, 2024

Uh oh!

chhagedorn commented Jun 26, 2024

Uh oh!

openjdk bot commented Jun 26, 2024

Uh oh!

openjdk bot commented Jun 26, 2024

Uh oh!

Uh oh!

chhagedorn commented Jun 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 24, 2024 •

edited

Loading

chhagedorn Jun 24, 2024 •

edited

Loading