RFR: 8329797: Shenandoah: Default case invoked for: "MaxL" (bad AD file)

[Aleksey Shipilev]

On Wed, 17 Apr 2024 18:44:57 GMT, Joshua Cao <duke at openjdk.org> wrote:

> The bug occurs when [Shenandoah optimizations resets post_loop_opts](https://github.com/openjdk/jdk/blob/040c93565c0dff6270911eb9e58d78aa01bbb925/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp#L52), and we may [create a MaxL after macro expansion](https://github.com/openjdk/jdk/blob/040c93565c0dff6270911eb9e58d78aa01bbb925/src/hotspot/share/opto/movenode.cpp#L198). `MaxL` does not have a matcher rule, and we run into an assertion failure.
> 
> This PR guards the `MaxL` creation with a new `began_macro_expansion()` flag. I think there are many other instances in code that should use the new flag instead of `post_loop_opts()`, which can be explored in [JDK-8330531](https://bugs.openjdk.org/browse/JDK-8330531).
> 
> The bug was originally found in [h2 Index::getCostRangeIndex()](https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/index/Index.java#L579) through Dacapo. Its easy to reproduce by creating a loop that includes a `ShenandoahLoadReferenceBarrier` (load any object) and a `MaxL`.
> 
> Caveat: I created test cases for both `MaxL` and `MinL` for completeness. The `MinL` test case does not actually fail before this PR. Somehow the `CMove` condition is converted to non-canonical `>`, which is [not accepted by the Idealization](https://github.com/openjdk/jdk/blob/040c93565c0dff6270911eb9e58d78aa01bbb925/src/hotspot/share/opto/movenode.cpp#L219). The `MinL` is never created and there is no crash.
> 
> Passing hotspot tier1 locally on Linux machine.

Good catch!

src/hotspot/share/opto/compile.hpp line 321:

> 319: 
> 320:   bool                  _post_loop_opts_phase;  // Loop opts are finished.
> 321:   bool                  _began_macro_expansion; // Macro expansion is started.

I think this sounds better as inverse, `bool _allow_macro_nodes; // Allow creating macro nodes`.  Then we can also assert `_allow_macro_nodes` in `add_macro_node`?

test/hotspot/jtreg/compiler/c2/irTests/TestIfMinMax.java line 44:

> 42:     public static void main(String[] args) {
> 43:         TestFramework.run();
> 44:         TestFramework.runWithFlags("-XX:+UseShenandoahGC");

I don't think we add GC-specific testing here. For one, the test would fail for the builds that do not include Shenandoah. 

The common practice it to rely on test pipelines running the test suites with different GCs. Does `make test TEST=compiler/c2/irTests/TestIfMinMax.java TEST_VM_OPTS=-XX:+UseShenandoahGC` work?

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

PR Review: https://git.openjdk.org/jdk/pull/18824#pullrequestreview-2010863520
PR Review Comment: https://git.openjdk.org/jdk/pull/18824#discussion_r1572033108
PR Review Comment: https://git.openjdk.org/jdk/pull/18824#discussion_r1572035101