RFR: JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late [v5]

[Christian Hagedorn]

On Fri, 20 Aug 2021 06:11:47 GMT, 王超 <github.com+25214855+casparcwang at openjdk.org> wrote:

>> Current loop predication will promote nodes(with a dependency on a later control node) to the insertion point which dominates the later control node.
>> 
>> In the following example, loopPrediction will promote node 434 to the outer loop(predicted insert point is right after node 424), and it depends on control node 207.  But node 424 dominates node 207, which means after the promotion, the cloned nodes have a control dependency on a later control node, which leads to a bad graph.
>> 
>> ![image](https://user-images.githubusercontent.com/25214855/129720970-ff65b8f4-8bef-401d-8590-54aca6de470e.png)
>> 
>> ![image](https://user-images.githubusercontent.com/25214855/129721369-4c61222b-7305-4522-9a37-e3e6e2138aa9.png)
>
> 王超 has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Prevent pinned loadi, and assert if found wrong loop predication

> > Given that we could fix this bug without a bail out of loop predication, either by my suggested fix or something else, I'd say that this check should never be taken. And if it is taken it indicates a missed range check elimination opportunity. But you're right, we cannot really prove that there are currently no other such cases. I would therefore suggest to either turn your bailout code into an assertion check or keep your bailout but add an `assert(false)` to catch a missed case in the future to further investigate. If we will at some point face a non-fixable case we can still remove the assertion code again. But of course, all of this only applies if we can fix the current bug without bailing out of loop predication.
> 
> Thank you very much! I have make the bailing out code into an assertion, and include your suggested fix in this patch.
> 

Thanks for examining the suggested fix more carefully and adapting your original bailout fix to it!

> The first thing I worry about is: change the control of load will lead to wrong code sequence of different nodes. But during the tests, I found no problems. For two different loads, if a `volatile` load `l1` is before another normal load `l2`, the later load `l2` will have a memory dependency on the `acquire memory bar` node of the `volatile` load `l1`, so the final code sequence will be guaranteed. For two normal loads, the disorder has no problem. So I can not find a scenario which leads to the wrong code generation of your suggested fix, please remind me if you find something, and I will keep on testing.
> 
> I have run tests with your suggested fix, and find no problem.

I'm currently not seeing a problem, either, but it could be that we are potentially missing something. So, it would definitely be good to have some other opinions on this code change. I suggest to have at least 2 other reviews on it. I will also submit some testing for the fix over the weekend.

src/hotspot/share/opto/loopPredicate.cpp line 674:

> 672:     if (phase->get_loop(predicate_proj) == phase->get_loop(offset_ctrl) &&
> 673:         phase->is_dominator(predicate_proj, offset_ctrl)) {
> 674:       assert(false, "willl lead to cyclic dependency");

"willl" -> "will". Maybe you can also add "missed range check elimination" to the assertion message to make it more clear what the purpose of this assert is.

src/hotspot/share/opto/memnode.cpp line 1629:

> 1627:       if (this->in(0) == region) {
> 1628:         if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {
> 1629:           x->set_req(0, mem->in(i)->in(0)); // Use same control as memory

Maybe add a comment here that this enables other optimizations such as loop predication which does not work if we directly pin the node to `in`.

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

PR: https://git.openjdk.java.net/jdk/pull/5142