RFR: 8326638: Crash in PhaseIdealLoop::remix_address_expressions due to unexpected Region instead of Loop

[Tobias Hartmann]

On Mon, 26 Feb 2024 09:07:54 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Adds a missing `_head->is_Loop()` check to handle the case when the loop head is not a `LoopNode` but a `RegionNode` because the loop is irreducible:
>> 
>> 
>> (rr) p n_loop->_head->dump(1)
>>   412 IfTrue === 411 [[ 345 ]] #1 !jvms: Test$Class1::Class1_method0 @ bci:286 (line 119)
>>   339 SafePoint === 336 1 340 1 1 344 290 1 1 1 1 291 1 1 293 294 295 1 320 297 1 298 299 300 301 302 1 303 1 [[ 345 ]] SafePoint !jvms: Test$Class1::Class1_method0 @ bci:307 (line 122)
>>   345 Region === 345 339 412 [[ 345 456 416 417 418 419 420 421 422 423 424 425 426 427 428 429 430 431 1861 1863 1865 1867 1882 1888 ]] #irreducible !jvms: Test$Class1::Class1_method0 @ bci:138 (line 101)
>> 
>> 
>> We then crash when invoking `LoopNode::skip_strip_mined` on a RegionNode or assert in debug. Unfortunately, I was not able to simply the test any further.
>> 
>> This is a regression from [JDK-8303511](https://bugs.openjdk.org/browse/JDK-8303511) in JDK 21 b13
>> 
>> Thanks,
>> Tobias
>
> src/hotspot/share/opto/loopopts.cpp line 586:
> 
>> 584:           Node* add1 = new AddPNode(n->in(1), n->in(2)->in(2), n->in(3));
>> 585:           // Stuff new AddP in the loop preheader
>> 586:           Node* entry = n_loop->_head->is_Loop() ? n_loop->_head->as_Loop()->skip_strip_mined(1)->in(LoopNode::EntryControl)
> 
> Should we add a sanity assert (which could also serve as an explaining comment) that in the `RegionNode` case, it must be an irreducible loop head?

I could do that but it would feel a bit arbitrary to put the assert only in this code, given that we have the same `is_Loop` check in a lot of other code as well. Maybe it would make sense to factor the checks into a separate method at some point.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18002#discussion_r1502555553