Request for reviews (S): 7042327: assert(opaq->outcnt() == 1 && opaq->in(1) == limit)

[Vladimir Kozlov]

I will push the fix without it since it is not needed. But having Opaque node on 
  unrolled loop limit may prevent some loop optimizations which happened before 
this fix. So it is mostly performance issue, I think. So I will do it later with 
out performance work for loops. I am sure it safe :)

Thanks,
Vladimir

Tom Rodriguez wrote:
> On May 5, 2011, at 7:10 PM, Vladimir Kozlov wrote:
> 
>> Thank you, Tom
>>
>> I ran into small problem with last fix during Match because one IfNode has Con as condition but was not optimized out. I added the missing push to IGVN worklist in adjust_check() and updated webrev. It happened because Opaque2 node I added prevented constant propagation early. So I want to add Identity() to Opaque2 node but would like to get your opinion before I do this. Opaque2 node was originally (before this fix) used only for guarding loop's trip-counter. But with my change the limit expression could be transformed into constant or final value of trip-count could be transformed to constant if loop was empty. So I think it is safe to do this Identity transformation since it is not trip-count any more. What do you think?
>>
>> +Node *Opaque2Node::Identity( PhaseTransform *phase ) {
>> +  return (in(1)->is_Con()) ? in(1) : this;
>> +}
> 
> I'm not totally sure about this.  I thought the purpose of Opaque node was to be opaque, so performing optimizations on them seems questionable.  Do you need to do it for your fix?  I'd be leery of pushing that without some nightly testing unless you are very sure of it.  The other change seems fine.
> 
> tom
> 
>> Thanks,
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> Looks good.
>>> tom
>>> On May 5, 2011, at 5:30 PM, Vladimir Kozlov wrote:
>>>> Thank you, Tom
>>>>
>>>> Thinking about what you said and, yes, such situation could happen. To be safe I need always put Opaque2 for limit for unrolled loop. I updated webrev.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> It seems ok.  Can't this situation occur as a result of other optimizations?  If limit is behind by some expressions that later gets optimized away it seems like the same configuration could happen.  Or isn't that possible?
>>>>> tom
>>>>> On May 5, 2011, at 4:03 PM, Vladimir Kozlov wrote:
>>>>>> http://cr.openjdk.java.net/~kvn/7042327/webrev
>>>>>>
>>>>>> Fixed 7042327: assert(opaq->outcnt() == 1 && opaq->in(1) == limit)
>>>>>>
>>>>>> New loop unrolling code calculate new_limit = limit-stride. If limit is
>>>>>> trip-counter (phi+stride) with the same stride from a previous loop then new_limit will be optimized to pre-incremented value: new_limit = phi.
>>>>>> reorg_offsets() optimization will create a separate Opaque2 node for each use of trip-counter (phi) and as result zero trip guard limit will be different from loop limit and it causes the assert to fail.
>>>>>>
>>>>>> Separate limit by Opaque2 node when calculating new limit for unroll if limit is an incremented variable from previous loop to avoid using pre-incremented value and reduce register pressure.
>>>>>> I also removed code which creates dead loops since limit is input to new_limit.
>>>>>>
>>>>>> Tested with failed case and CTW.
>