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

[Tobias Hartmann]

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.

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

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.

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

For now, I just added an assert to CountedLoopEndNode::loopnode().

New webrev:
http://cr.openjdk.java.net/~thartmann/8134739/webrev.01/

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