Request for reviews (L): 7004535: Clone loop predicates when loop is cloned

[Vladimir Kozlov]

Tom Rodriguez wrote:
> Why are the extra checks being removed?
> 
> -     if (out == NULL || out->is_Root() || out->is_Start())
>         return false;
> 
> Are you just filtering those later along with other non Regions?

Yes, I assume only regions are expected on the path to uncommon trap calls. So I 
replaced checks for Root and Start with check for Region below otherwise it does 
more scanning than needed in false case:

+     if (out->Opcode() != Op_Region)
+       return false;

> 
> Why not just use the ifdef ASSERT version all the time instead of having two copies?

OK.

> 
> + #ifdef ASSERT
> +   if (is_uncommon_trap_proj(other_proj, reason)) {
> +     assert(reason == Deoptimization::Reason_none ||
> +            Compile::current()->is_predicate_opaq(iff->in(1)->in(1)), "should be on the list");
> +     return true;
> +   }
> +   return false;
> + #else
>     return is_uncommon_trap_proj(other_proj, reason);
> + #endif
> 
> Could you use a better variable name than "pj", maybe proj_index.

OK.

> 
> Extra declaration of predicate.
> 
> + Node* PhaseIdealLoop::skip_loop_predicates(Node* entry) {
> +   Node* predicate = NULL;
> +   if (UseLoopPredicate) {
> +     Node* predicate = 

Fixed.

> 
> The rest looks fine.

Thank you, Tom

Vladimir

> 
> tom
> 
> On Mar 29, 2011, at 9:33 AM, Vladimir Kozlov wrote:
> 
>> Thank you, Tom
>>
>> Tom Rodriguez wrote:
>>> ifnode.cpp:
>>> This appears to be unused.
>>> +   bool counted_loop = r->is_CountedLoop();
>> Remoived.
>>
>>> loopUnswitch.cpp:
>>> ! //    endif                        predicat [clone]
>> Fixed.
>>
>>> Unused?
>>> +   bool counted_loop = head->is_CountedLoop();
>> Remoived.
>>
>>> How does the code in loopPredicate.cpp compare to what was originally in loopTransform.cpp?  Are there changes or is it a straight copy?
>> No, I added a lot to do cloning. Here are diffs vs loopTransform.cpp:
>>
>> http://cr.openjdk.java.net/~kvn/7004535_1/webrev/
>>
>> Vladimir
>>
>>> tom
>>> On Mar 28, 2011, at 5:06 PM, Vladimir Kozlov wrote:
>>>> I still need reviews for this. I merged with latest changes and added new method skip_loop_predicates() used in policy_do_remove_empty_loop().
>>>>
>>>> http://cr.openjdk.java.net/~kvn/7004535/webrev.01
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Vladimir Kozlov wrote:
>>>>> Second loop opts changes.
>>>>> http://cr.openjdk.java.net/~kvn/7004535/webrev
>>>>> Fixed 7004535: Clone loop predicates when loop is cloned
>>>>> Currently loop predicates generated during parsing could be separated from loops when loops are cloned. As result such predicates are removed. There could be more optimization opportunities if loop predicates are also cloned in such situations.
>>>>> Loop predicate code become big so I moved it into new file loopPredicate.cpp.
>>>>> I separated the cloning code for IdealLoop and IterGVN. The generated Ideal
>>>>> code is the same but registration of new Ideal nodes is different enough to
>>>>> have separate methods, I think. I welcome any suggestions to improve this code.
>>>>> Keep loop predicates after CCP and perform optimizations with them until no more
>>>>> loop optimizations could be done. After that switch them off and do more loop
>>>>> optimizations.
>>>>> VectorNode missed size_of() method as result it was cloned incorrectly.
>>>>> Added TraceLoopOpts outputs I missed in previous changes.
>>>>> Tested: ctw, nsk, jprt
>