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

Fri Apr 19 16:54:59 UTC 2024

On Fri, 19 Apr 2024 16:47:08 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.
>
> Joshua Cao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update comment for MinL/MaxL based on renaming of allow_macro_nodes

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

> 790: 
> 791:   bool       allow_macro_nodes() { return _allow_macro_nodes;  }
> 792:   void  dont_allow_macro_nodes() { _allow_macro_nodes = false;  }

`dont_allow_macro_nodes` is a confusing name here, especially given it is a setter in contrast to `allow_macro_nodes`. Let's call it `reset_allow_macro_nodes()`.

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

> 2446: //  Returns true if a failure occurred.
> 2447: bool PhaseMacroExpand::expand_macro_nodes() {
> 2448:   C->dont_allow_macro_nodes();