Request for reviews (L): 7008866: Missing loop predicate for loop with multiple entries

[Vladimir Kozlov]

Tom Rodriguez wrote:
> is_canonical_counted reads weird.  Maybe is_valid_counted_loop?

OK.

> 
> Do the intrinsics really benefit from having predicates?  Most those assume that Strings are well formed and don't have range checks.

For loop limit check fix I will use the same mechanism. So each loop should have 
a predicate, otherwise it will not be converted into CountedLoop since we can't 
insert a predicate check during ideal graph transformation (they are added 
during parsing to get correct jvm state).

> 
> If you have the urge, a lot of the code that simply moved could have its "if( " whitespace corrected.

You mean, to use "if (a) {" instead of "if( a )" ? OK. I am actually thinking to 
move all predicate code into separate file loopPredicate.cpp in predicates 
cloning changes.

> 
> loopnode.cpp:
> 
> Is this no longer needed or did it more somewhere?
> 
>     _igvn.hash_delete(phi);
>     phi->set_req_X( LoopNode::LoopBackControl, incr, &_igvn );
>   
>     // If phi type is more restrictive than Int, raise to
>     // Int to prevent (almost) infinite recursion in igvn
>     // which can only handle integer types for constants or minint..maxint.
>     if (!TypeInt::INT->higher_equal(phi->bottom_type())) {
>       Node* nphi = PhiNode::make(phi->in(0), phi->in(LoopNode::EntryControl), TypeInt::INT);
>       nphi->set_req(LoopNode::LoopBackControl, phi->in(LoopNode::LoopBackControl));
>       nphi = _igvn.register_new_node_with_optimizer(nphi);
>       set_ctrl(nphi, get_ctrl(phi));
> +   nphi->set_req(LoopNode::LoopBackControl, incr);

I want to avoid side effects of set_req_X() call so I am always replacing old 
phi with new one:

+   Node* nphi = PhiNode::make(x, init_trip, TypeInt::INT);
+   nphi = _igvn.register_new_node_with_optimizer(nphi);
+   set_ctrl(nphi, get_ctrl(phi));

> 
> loopTransform.cpp:
> 
> !   if (must_reason_predicate) {
> !   if (reason != Deoptimization::Reason_none) {
> 
> Shouldn't that really be reason == Deoptimization::Reason_predicate?  That the only one that expects the opaque node.

Limit check will have different reason Deoptimization::Reason_loop_limit_check 
and I want to reuse this code for it.

> 
> Why does create_new_if_for_predicate have the two extra arguments?  They are always passed as NULL, Deoptimization::Reason_predicate.

They will be used for predicates cloning and loop limit check. I am sorry, I cut 
these changes from much larger changes which include all work I did before. This 
is why they have this code.

> 
> Overall it looks fine to me.  That factoring and sharing of logic is a nice improvement.

I also wanted to share range check code from do_range_check() and rc_predicate() 
but I need more time for it.

Thanks,
Vladimir

> 
> tom
> 
> On Mar 14, 2011, at 2:54 PM, Vladimir Kozlov wrote:
> 
>> It is the first of coming loop opts changes. No benchmarks performance improvement is expected since main loops in our benchmarks have predicates already.
>>
>> http://cr.openjdk.java.net/~kvn/7008866/webrev
>>
>> Fixed 7008866: Missing loop predicate for loop with multiple entries
>>
>> Synopsis is a little misleading since I don't mean irreducible loop entries
>> (new code has a check to not add predicates to irreducible loops). I mean there
>> could be several entry and backbranch paths to loop head.
>>
>> Currently loop predicates are generated when each backbranch path is parsed. Later in IdealLoopTree::beautify_loops() a merge region will be created if there are several entry paths separating predicates from the loop. As result those predicates will be removed.
>>
>> Add predicates when loop head bytecode is parsed instead of when back branch
>> bytecode is parsed. All fall in paths will be merged already at this point and
>> for back branches a new merge region is created.
>>
>> Add missing loop predicate when there are back branches to the method entry
>> first bytecode.
>>
>> Add missing predicates for loops generated in Ideal graph, for example, for
>> intrinsics.
>>
>> Rearrange some code to help next loop optimization changes. For example, in
>> PhaseIdealLoop::is_counted_loop() move bailout checks before new ideal nodes are
>> generated. Add new counted loop verification code.
>>
>> Perform local Ideal transformation for parallel induction variables replacement
>> since in most cases they use the same stride (ratio == 1).
>>
>> In PhaseIdealLoop::split_thru_phi() don't move split node (or existing node
>> returned by Identity()) inside a loop if all its uses are outside of this loop.
>> Otherwise the node will be cloned for each outside use and this will mess up an
>> external canonical counted loop if the node is loop's increment, for example.
>>
>> Preserve unswitch count when partial peel is executed and when a loop is
>> converted to counted.
>>
>> Add debug flag TraceLoopOpts to show executed loop optimizations and also print
>> whole loop tree at the beginning of each iteration of loop optimizations:
>>
>> PartialPeel   Loop: N22966/N22965
>>
>> Tested: ctw, nsk, jprt
>> Ran refworkload on x86 - nor regression (or improvement).
>>
>