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

Tue Sep 15 15:04:54 UTC 2015

Go with your fix: checking for a removed Opaque1Node. It is reasonable fix.
Thank you for verifying that bailout also works. It requires additional round of loopopts so you fix is less expensive.

Thanks,
Vladimir

On 9/15/15 6:54 AM, Tobias Hartmann wrote:
> 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
>>>>>>