[9] RFR(S): 8134739: compiler/loopopts/superword/TestVectorizationWithInvariant crashes in loop opts

[Tobias Hartmann]

Hi Vladimir,

On 14.09.2015 17:44, Vladimir Kozlov wrote:
> On 9/14/15 3:16 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review!
>>
>> On 11.09.2015 06:12, Vladimir Kozlov wrote:
>>> Did remove_main_post_loops() change something to main loop?
>>
>> Yes, it removes the zero-trip Opaque1Node of the main loop so it can be optimized out. I missed that.
>>
>>> Returning NULL may be not safe because not all places checks that phi() != NULL.
>>
>> Yes, but phi() may already return NULL even without my changes.
> 
> And I am saying it is bug that if we don't check in places calling cl->phi() for NULL.
> You returning NULL may increase chances that we will hit problems.

Yes, I agree.

>>> I think we should bail-out this round of loopopts by returning false:
>>>    if (policy_do_remove_empty_loop(phase)) {
>>>      // Here we removed an empty loop.
>>>      // Bail out this round of loopopts - we need to fully collapse
>>>      // this loop (during IGVN) before proceeding further.
>>>      return false;
>>>    }
>>
>> We already do that but we only bail out for the pre-loop and then continue with the main loop.
> 
> If we return 'false' instead of current 'true' it will bail out from loop opts without processing following loops. See caller IdealLoopTree::iteration_split(). Then CountedLoopEnd should be gone after IGVN and the check is_CountedLoopEnd() in SuperWord::get_pre_loop_end(). May be it is not removed for some reason - please, check.

Right, I missed that you changed the return value to 'false'. I just checked and bailing out works fine - the CountedLoopEndNode is removed.

>> I think we should check if the Opaque1Node of the main loop was removed like we do in "PhaseIdealLoop::do_range_check":
>>
>>    // Can not optimize a loop if zero-trip Opaque1 node is optimized
>>    // away and then another round of loop opts attempted.
>>    if (opqzm->Opcode() != Op_Opaque1)
>>      return;
>>
>> I changed the fix accordingly.
> 
> Yes, it could be solution too.

I verified that both solutions (bailing out and checking for a removed Opaque1Node) work fine. Which solution do you prefer?

>>> Also your change could be useful after adjustment (execute assert):
>>>
>>> tmp->in(1)->as_Phi()
>>>
>>> You may need to run more testing before pushing to see if you hit this assert.
>>
>> Right, but then we fail at other places (for example, PhiNode::Value()) where we expect phi() to return "something" even if the loop does not contain a PhiNode. We would have to adapt all call sites to check if the loop is in a state where it contains a PhiNode or return NULL and handle it. What do you think?
> 
> File separate bug. We need to fix places where we assume that Phi node is returned.

I filed JDK-8136547.

>> For now, I just added an assert to CountedLoopEndNode::loopnode().
>>
>> New webrev:
>> http://cr.openjdk.java.net/~thartmann/8134739/webrev.01/
> 
> This look good. But first, please, check if CountedLoopEnd is removed by IGVN.

I did and the CountedLoopEndNode is removed by IGVN.

Best,
Tobias


> Thanks,
> Vladimir
> 
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 9/9/15 4:56 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> Roland asked (off-thread) why we still optimize the empty loop after replacing the PhiNode to collapse it. Here is a more detailed trace of events to clarify this. We have the following loops:
>>>>
>>>>    Loop: N2433/N2436  limit_check predicated counted [0,int),+8 (4 iters)  pre
>>>>    Loop: N2484/N1546  counted [int,92),+16 (-2147483648 iters)  main
>>>>    Loop: N2388/N2391  counted [int,100),+8 (4 iters)  post
>>>>
>>>> IdealLoopTree::iteration_split() first looks a the pre-loop and removes it because it's empty:
>>>>
>>>>    IdealLoopTree::iteration_split
>>>>    IdealLoopTree::iteration_split_impl
>>>>    IdealLoopTree::policy_do_remove_empty_loop
>>>>
>>>>    Empty with zero trip guard       Loop: N2433/N2436  limit_check predicated counted [0,100),+8 (4 iters)  pre
>>>>
>>>> We do not continue optimizing the pre-loop but bail out when returning from IdealLoopTree::policy_do_remove_empty_loop() and continue with the main-loop:
>>>>
>>>>    IdealLoopTree::iteration_split
>>>>    IdealLoopTree::iteration_split_impl
>>>>    IdealLoopTree::policy_unroll
>>>>    IdealLoopTree::policy_unroll_slp_analysis
>>>>    SuperWord::transform_loop(IdealLoopTree* lpt)
>>>>    SuperWord::get_pre_loop_end(CountedLoopNode* cl)
>>>>
>>>>    // Check for pre-loop ending with CountedLoopEnd(Bool(Cmp(x,Opaque1(limit))))
>>>>
>>>> The SuperWordLoopUnrollAnalysis now looks at the pre-loop again and fails because the PhiNode was replaced.
>>>>
>>>> I noticed that the output of IdealLoopTree::dump_head() sometimes contains negative trip counts ("-2147483648 iters"). I filed JDK-8135252.
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>> On 07.09.2015 14:03, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8134739
>>>>> http://cr.openjdk.java.net/~thartmann/8134739/webrev.00
>>>>>
>>>>> Problem:
>>>>> The VM crashes during loop optimizations in CountedLoopEndNode::loopnode() because CountedLoopEndNode::phi() does not return a PhiNode but a SubINode. The problem is that the phi (2432 PhiNode) was replaced by the final value of the last loop iteration (2881 SubINode) to collapse the empty loop in IdealLoopTree::policy_do_remove_empty_loop(). See before [1] and after [2].
>>>>>
>>>>> Solution:
>>>>> CountedLoopEndNode::phi() should check if the node really is a PhiNode and return NULL otherwise.
>>>>>
>>>>> Testing:
>>>>> - Failing test
>>>>> - JPRT
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/secure/attachment/52878/emptyLoop1.png
>>>>> [2] https://bugs.openjdk.java.net/secure/attachment/52879/emptyLoop2.png
>>>>>