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

[Tom Rodriguez]

On Mar 18, 2011, at 5:00 PM, Vladimir Kozlov wrote:

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

Ok.

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

Yes.  That sounds good.

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

Ok, I missed the earlier wide type.

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

Ok.

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

That's fine.

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

Smaller pieces at a time is probably best.  Looks good.

tom

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