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

Tue Sep 15 16:36:40 UTC 2015

Thanks, Vladimir! I go with the "check for removed Opaque1Node" fix then.

Best,
Tobias

On 15.09.2015 17:04, Vladimir Kozlov wrote:
> 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
>>>>>>>