RFR: 8317349: Randomize order of macro node expansion in C2

[Christian Hagedorn]

On Fri, 2 Feb 2024 13:03:53 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

> 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 new Ideal phases related to macro expansion, allowing `StressMacroExpansion` testing. Below is a sample phase list screenshot including some of the new phases (IGV print level 4).
>    <p align="center">
>       <img src="https://github.com/openjdk/jdk/assets/4222397/b65c67a0-13f7-45a6-aef4-5dfe259dab66" width="300">
>    </p>
> 
> 
> Question for the review: are the new macro expansion Ideal phases OK, or should we change anything?
> 
> Testing:
> - [GitHub Actions](https://github.com/dlunde/jdk/actions/runs/7756242938)
> - 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.

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

src/hotspot/share/opto/compile.cpp line 5126:

> 5124: }
> 5125: 
> 5126: // Shuffle macro nodes

I think you can remove this comment as it should be clear from the method name.

src/hotspot/share/opto/compile.cpp line 5128:

> 5126: // Shuffle macro nodes
> 5127: void Compile::shuffle_macro_nodes() {
> 5128:   if (_macro_nodes.length() < 2) return;

I suggest to add braces:
Suggestion:

  if (_macro_nodes.length() < 2) {
    return;
  }

src/hotspot/share/opto/macro.cpp line 2438:

> 2436:   if (StressMacroExpansion) {
> 2437:     C->shuffle_macro_nodes();
> 2438:   }

I suggest to do this already before `eliminate_macro_nodes()` to also get a different macro node elimination order if multiple nodes could be removed.

src/hotspot/share/opto/macro.cpp line 2517:

> 2515:       assert(!success || (C->macro_count() == (old_macro_count - 1)), "elimination must have deleted one node from macro list");
> 2516:       progress = progress || success;
> 2517:       if (success) { C->print_method(PHASE_AFTER_MACRO_EXPANSION_ELIMINATE, 4, n); }

I suggest to put it on a new line. 
Suggestion:

      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.

test/hotspot/jtreg/compiler/c2/TestModDivTopInput.java line 31:

> 29:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP -XX:+StressIGVN -XX:+StressMacroExpansion
> 30:  *                   -Xcomp -XX:CompileOnly=TestModDivTopInput::* -XX:-TieredCompilation -XX:StressSeed=87628618 TestModDivTopInput
> 31:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP -XX:+StressIGVN -XX:+StressMacroExpansion

I think we should not add this option here as this test was written as a regression test that should trigger a specific bug with the originally provided flags.

test/hotspot/jtreg/compiler/loopopts/TestPeelingSkeletonPredicateInitialization.java line 35:

> 33:  *             we do not statically realize that the peeled loop can never be entered.
> 34:  *
> 35:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP -XX:+StressIGVN -XX:+StressMacroExpansion

Same here, I would not add this option here since it's a regression test for a specific bug.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17684#pullrequestreview-1862869511
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478295898
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478295522
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478309545
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478305556
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478313313
PR Review Comment: https://git.openjdk.org/jdk/pull/17684#discussion_r1478335110