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

[Vladimir Kozlov]

Did remove_main_post_loops() change something to main loop?
Returning NULL may be not safe because not all places checks that phi() 
!= NULL. 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;
   }

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.

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
>>