[9] RFR(S): 8134739: compiler/loopopts/superword/TestVectorizationWithInvariant crashes in loop opts
Vladimir Kozlov
vladimir.kozlov at oracle.com
Mon Sep 14 15:44:37 UTC 2015
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.
>
>> 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.
>
> 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.
>
>> 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.
>
> 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.
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
>>>>
More information about the hotspot-compiler-dev
mailing list