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

[Tom Rodriguez]

is_canonical_counted reads weird.  Maybe is_valid_counted_loop?

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

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

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

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.

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

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

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