8317349: Randomize order of macro node expansion in C2 #17684

dlunde · 2024-02-02T13:03:53Z

This changeset adds a flag StressMacroExpansion which randomizes macro expansion ordering for stress testing purposes.

Changes:

Implement the flag StressMacroExpansion.
Add functionality tests for StressMacroExpansion, analogous to testing for previous stress flags such as StressIGVN.
Add StressMacroExpansion to a number of tests that already turn on all other stress flags (and therefore should also use StressMacroExpansion).
Rename the previous compiler phase MACRO_EXPANSION to AFTER_MACRO_EXPANSION.
Add a new Ideal phase for individual macro expansion steps, allowing StressMacroExpansion testing. Below is a sample phase list screenshot including the new phase (IGV print level 5 with "After Iter GVN Step" omitted).

Question for the review: are the new macro expansion Ideal phases OK, or should we change anything?

Testing:

GitHub Actions
tier1 to tier5 on windows-x64, linux-x64, linux-aarch64, macosx-x64, and macosx-aarch64.
Tested that thousands of graphs are correctly opened and visualized with IGV.

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-8317349: Randomize order of macro node expansion in C2 (Enhancement - P4)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Roberto Castañeda Lozano (@robcasloz - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17684

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-02T13:04:17Z

👋 Welcome back dlunden! 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-02-02T13:04:51Z

@dlunde 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 · 2024-02-02T13:07:11Z

Webrevs

chhagedorn

That's a nice stress option! I have a few comments.

src/hotspot/share/opto/compile.cpp

chhagedorn · 2024-02-05T14:02:55Z

src/hotspot/share/opto/macro.cpp

@@ -2511,6 +2514,7 @@ bool PhaseMacroExpand::expand_macro_nodes() {
      }
      assert(!success || (C->macro_count() == (old_macro_count - 1)), "elimination must have deleted one node from macro list");
      progress = progress || success;
+      if (success) { C->print_method(PHASE_AFTER_MACRO_EXPANSION_ELIMINATE, 4, n); }


I suggest to put it on a new line.

Suggested change

if (success) { C->print_method(PHASE_AFTER_MACRO_EXPANSION_ELIMINATE, 4, n); }

if (success) {

C->print_method(PHASE_AFTER_MACRO_EXPANSION_ELIMINATE, 4, n);

}

I think dumping at level 4 (same for the other newly added dumps) is too verbose already but when dumping at level 5, you have the overhead of additional IGVN step dumps. I could imagine that you either want macro expansion dumps OR IGVN step dumps but not always both. So, it looks like we've now reached a point where it would have been nice to enable/disable certain dumps.

Anyway, since this is not possible I suggest to move these dumps to level 5 for now. But I'm open for other opinions about that.

I think we should also add this to eliminate_macro_nodes() when eliminating locks, allocations, and boxing nodes. But then again, we also dump the graphs directly after EA since eliminate_macro_nodes() is also called from there. Not sure if that's wanted. I guess it's okay being at IGV dump level 5. But we could also restrict the dumps to the post loop opts phase.

src/hotspot/share/opto/macro.cpp

test/hotspot/jtreg/compiler/c2/TestModDivTopInput.java

test/hotspot/jtreg/compiler/loopopts/TestPeelingSkeletonPredicateInitialization.java

robcasloz

Looks good to me, modulo Christian's comments. Regarding whether to randomize macro node elimination, I find it a bit confusing that PhaseMacroExpand also eliminates macro nodes, perhaps it would be better to, in a separate RFE, split the phase into PhaseMacroEliminate and PhaseMacroExpand, and introduce a separate StressMacroElimination option and new phase names for the former?

dlunde · 2024-02-05T15:51:43Z

Thanks @chhagedorn and @robcasloz. I made the trivial changes suggested by Christian, so the remaining question is how to handle dump levels and what/when to dump.

My suggestion: set dump levels to 5 for now, as Christian suggests, but do not yet add any dumps to eliminate_macro_nodes. I'll create a separate RFE with Roberto's suggestion (which I think sounds good and, looking at the code, quite easy to achieve through a bit of restructuring) together with IGV dumping for macro elimination. What do you think?

chhagedorn

Thanks for the updates!

Regarding whether to randomize macro node elimination, I find it a bit confusing that PhaseMacroExpand also eliminates macro nodes, perhaps it would be better to, in a separate RFE, split the phase into PhaseMacroEliminate and PhaseMacroExpand, and introduce a separate StressMacroElimination option and new phase names for the former?

I agree with that. A new phase would be better. Let's do that in a separate RFE.

My suggestion: set dump levels to 5 for now, as Christian suggests, but do not yet add any dumps to eliminate_macro_nodes. I'll create a separate RFE with Roberto's suggestion (which I think sounds good and, looking at the code, quite easy to achieve through a bit of restructuring) together with IGV dumping for macro elimination. What do you think?

Sounds good!

test/hotspot/jtreg/compiler/loopopts/TestPeelingSkeletonPredicateInitialization.java

openjdk · 2024-02-06T09:34:20Z

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

8317349: Randomize order of macro node expansion in C2

Reviewed-by: chagedorn, rcastanedalo, 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 145 new commits pushed to the master branch:

3bffe22: 8319463: ClassSignature should have superclass and superinterfaces as ClassTypeSig
e0d98dd: 8325257: jshell reports NoSuchFieldError with instanceof primitive type
f2f6344: 8325347: Rename native_thread.h
1797efd: 8322218: Better escaping of single and double quotes in annotation toString() results
0f5f3c9: 8325254: CKA_TOKEN private and secret keys are not necessarily sensitive
4b1e367: 8325152: Clarify specification of java.io.RandomAccessFile.setLength
96eb039: 8324665: Loose matching of space separators in the lenient date/time parsing mode
2d252ee: 8325180: Rename jvmti_FollowRefObjects.h
b814c31: 8321703: jdeps generates illegal dot file containing nodesep=0,500000
50b17d9: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers
... and 135 more: https://git.openjdk.org/jdk/compare/6d36eb78ad781ecd80d66d1319921a8746820394...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 (@chhagedorn, @robcasloz, @TobiHartmann) 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).

robcasloz

Looks good!

src/utils/IdealGraphVisualizer/README.md

src/hotspot/share/opto/macro.cpp

TobiHartmann

Nice! The changes look good to me.

vnkozlov · 2024-02-06T18:19:30Z

Looks good to me, modulo Christian's comments. Regarding whether to randomize macro node elimination, I find it a bit confusing that PhaseMacroExpand also eliminates macro nodes, perhaps it would be better to, in a separate RFE, split the phase into PhaseMacroEliminate and PhaseMacroExpand, and introduce a separate StressMacroElimination option and new phase names for the former?

Yes, I agree with this.

dlunde · 2024-02-07T09:08:29Z

I've now rerun tests and this is ready for integration. Thanks for the reviews, please sponsor!

/integrate

openjdk · 2024-02-07T09:09:24Z

@dlunde
Your change (at version 660cc89) is now ready to be sponsored by a Committer.

robcasloz · 2024-02-07T10:01:05Z

/sponsor

openjdk · 2024-02-07T10:01:37Z

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

3bffe22: 8319463: ClassSignature should have superclass and superinterfaces as ClassTypeSig
e0d98dd: 8325257: jshell reports NoSuchFieldError with instanceof primitive type
f2f6344: 8325347: Rename native_thread.h
1797efd: 8322218: Better escaping of single and double quotes in annotation toString() results
0f5f3c9: 8325254: CKA_TOKEN private and secret keys are not necessarily sensitive
4b1e367: 8325152: Clarify specification of java.io.RandomAccessFile.setLength
96eb039: 8324665: Loose matching of space separators in the lenient date/time parsing mode
2d252ee: 8325180: Rename jvmti_FollowRefObjects.h
b814c31: 8321703: jdeps generates illegal dot file containing nodesep=0,500000
50b17d9: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers
... and 135 more: https://git.openjdk.org/jdk/compare/6d36eb78ad781ecd80d66d1319921a8746820394...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-07T10:01:44Z

@robcasloz @dlunde Pushed as commit 4abb10e.

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

Add StressMacroExpansion

Loading
Loading status checks…

0e6349c

openjdk bot changed the title ~~8317349~~ 8317349: Randomize order of macro node expansion in C2 Feb 2, 2024

openjdk bot added the rfr label Feb 2, 2024

openjdk bot added the hotspot-compiler label Feb 2, 2024

chhagedorn reviewed Feb 5, 2024

View reviewed changes

robcasloz reviewed Feb 5, 2024

View reviewed changes

dlunde added 2 commits February 5, 2024 16:33

Updates after Christian's comments

Loading
Loading status checks…

96b01e7

Remove flag from some test cases

Loading
Loading status checks…

8ed2252

chhagedorn approved these changes Feb 6, 2024

View reviewed changes

test/hotspot/jtreg/compiler/loopopts/TestPeelingSkeletonPredicateInitialization.java Outdated Show resolved Hide resolved

openjdk bot added the ready label Feb 6, 2024

Update to print level 5 and fix copyright

Loading
Loading status checks…

f1b12e2

robcasloz approved these changes Feb 6, 2024

View reviewed changes

chhagedorn reviewed Feb 6, 2024

View reviewed changes

src/utils/IdealGraphVisualizer/README.md Outdated Show resolved Hide resolved

src/hotspot/share/opto/macro.cpp Outdated Show resolved Hide resolved

Change to single macro expansion step phase

Loading
Loading status checks…

660cc89

chhagedorn approved these changes Feb 6, 2024

View reviewed changes

TobiHartmann approved these changes Feb 6, 2024

View reviewed changes

robcasloz approved these changes Feb 6, 2024

View reviewed changes

openjdk bot added the sponsor label Feb 7, 2024

openjdk bot added the integrated label Feb 7, 2024

openjdk bot closed this Feb 7, 2024

openjdk bot removed ready rfr sponsor labels Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8317349: Randomize order of macro node expansion in C2 #17684

8317349: Randomize order of macro node expansion in C2 #17684

dlunde commented Feb 2, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 2, 2024

openjdk bot commented Feb 2, 2024

mlbridge bot commented Feb 2, 2024 •

edited

Loading

chhagedorn left a comment

chhagedorn Feb 5, 2024

chhagedorn Feb 5, 2024

robcasloz left a comment

dlunde commented Feb 5, 2024

chhagedorn left a comment •

edited

Loading

openjdk bot commented Feb 6, 2024 •

edited

Loading

robcasloz left a comment

TobiHartmann left a comment

vnkozlov commented Feb 6, 2024

dlunde commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

robcasloz commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

8317349: Randomize order of macro node expansion in C2 #17684

8317349: Randomize order of macro node expansion in C2 #17684

Conversation

dlunde commented Feb 2, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 2, 2024

openjdk bot commented Feb 2, 2024

mlbridge bot commented Feb 2, 2024 • edited Loading

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Feb 5, 2024

Choose a reason for hiding this comment

chhagedorn Feb 5, 2024

Choose a reason for hiding this comment

robcasloz left a comment

Choose a reason for hiding this comment

dlunde commented Feb 5, 2024

chhagedorn left a comment • edited Loading

Choose a reason for hiding this comment

openjdk bot commented Feb 6, 2024 • edited Loading

robcasloz left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

vnkozlov commented Feb 6, 2024

dlunde commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

robcasloz commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

dlunde commented Feb 2, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 2, 2024 •

edited

Loading

chhagedorn left a comment •

edited

Loading

openjdk bot commented Feb 6, 2024 •

edited

Loading