RFR(M): 8136725 Provide utility for creation a counted loop reserve copy (clone)

[Civlin, Jan]

Thank you, Vladimir.

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Thursday, October 08, 2015 8:14 PM
To: Civlin, Jan; Igor Veresov
Cc: 'hotspot compiler'
Subject: Re: RFR(M): 8136725 Provide utility for creation a counted loop reserve copy (clone)

Looks good. Igor, please, use hotspot.patch which does not have .hgtag. 
Don't forgot to add Contributed-by: jan.civlin at intel.com

http://cr.openjdk.java.net/~mcberg/8136725/webrev.03/hotspot.patch

Thanks,
Vladimir

On 10/9/15 7:14 AM, Civlin, Jan wrote:
> Vladimir, Igor,
>
> The webrev was uploaded to this location:
>
> http://cr.openjdk.java.net/~mcberg/8136725/webrev.03/
>
>
> -----Original Message-----
> From: Civlin, Jan
> Sent: Tuesday, October 06, 2015 1:44 AM
> To: 'Vladimir Kozlov'
> Cc: 'hotspot compiler'; Civlin, Jan
> Subject: RE: RFR(M): 8136725 Provide utility for creation a counted 
> loop reserve copy (clone)
>
> Seems the attached file was stripped by firewall. Trying to send again.
>
> -----Original Message-----
> From: Civlin, Jan
> Sent: Monday, October 5, 2015 6:54 PM
> To: Vladimir Kozlov
> Cc: hotspot compiler; Civlin, Jan
> Subject: RE: RFR(M): 8136725 Provide utility for creation a counted 
> loop reserve copy (clone)
>
> Vladimir, thank you for the review.
>
> Here is the updated patch.
>
> I decided to stick to simple if(TraceLoopOpts) condition for checking in printing, since these messages are printed on error only.
>
>
> Regards,
>
> Jan.
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Monday, October 05, 2015 1:08 AM
> To: Civlin, Jan
> Cc: hotspot compiler
> Subject: Re: RFR(M): 8136725 Provide utility for creation a counted 
> loop reserve copy (clone)
>
> remove .hgtags changes
>
> Trace outputs in create_reserve() are not under trace flag. I would suggest to remove them or put under TraceLoopOpts && Verbose combination.
> Also, please, split next and similar lines according to our code stile:
>
> if (!_lp_reserved->is_CountedLoop()) return false;
> ---
> if (!_lp_reserved->is_CountedLoop()) {
>     return false;
> }
>
> The rest looks fine.
>
> Thanks,
> Vladimir
>
> On 10/1/15 4:31 AM, Igor Veresov wrote:
>> Here’s the updated webrev:
>>
>> http://cr.openjdk.java.net/~iveresov/loop-reverse/webrev.02/
>>
>> igor
>>
>>> On Sep 28, 2015, at 2:25 AM, Civlin, Jan <jan.civlin at intel.com 
>>> <mailto:jan.civlin at intel.com>> wrote:
>>>
>>> Vladimir,
>>>
>>> Thank you for the review.
>>> I'm attaching the patch reflecting your comments.
>>>
>>> Regards,
>>>
>>> Jan
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Wednesday, September 23, 2015 3:38 AM
>>> To: Civlin, Jan; hotspot compiler
>>> Subject: Re: RFR(M): 8136725 Provide utility for creation a counted 
>>> loop reserve copy (clone)
>>>
>>> in loopUnswitch.cpp debug output should be under nonproduct flag 
>>> (could be TraceLoopOpts or separate new flag). Use #ifndef PRODUCT 
>>> around print code instead of repeated NOT_PRODUCT() macro. And I 
>>> don't think you need all output you added in final version.
>>>
>>> All constants are IGVNed so you don't need to pre-generate const_0.
>>> Since it is not connected to graph it could be eliminated.
>>> And I don't think you need next for the same reason:
>>>
>>>   311   lk->set_const_0(const_0);
>>>   312   lk->set_const_1(const_1);
>>>
>>> In ~CountedLoopReserveKit() and switch_to_reserved() you can make 
>>> new
>>> const_0 since you have pointer to IGVN:
>>>
>>> _phase->_igvn.intcon(0);
>>>
>>> You can't use subsume() since it will replace all users of const_1:
>>>
>>> +    //TODO bug? could not use here _const_1->subsume_by(_const_0,
>>> _phase->C);
>>>
>>> It is correct to use set_req() for constants here.
>>>
>>> superword.cpp I don't think you need to keep 
>>> _CountedLoopReserveKit_test code in final version after you verified it works during development:
>>>
>>> +  //!///    Testing that switching to reserved copy works
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 9/19/15 1:05 AM, Civlin, Jan wrote:
>>>> As I was working on modification of SuperWord::output - and this is 
>>>> a function where you modify the ideal graph on the fly and cannot 
>>>> exit on error - I was wondering how can I protect my algorithm in a 
>>>> case some wrong decision was made on the previous stage of the 
>>>> SuperWord but discovered only at the final stage, when the graph is 
>>>> already half modified.
>>>>
>>>> So here is my solution:
>>>> - the graph to be modified is cloned.
>>>> - five nodes are added: intcon(0) is disconnected, intcon(1)->If->
>>>> IfTrue->orig_loop: IfFalse->clone_loop.
>>>>
>>>> Then any optimization is executed on the original loop, and if it 
>>>> finishes ok, nothing else need to be done, the clone_loop will be 
>>>> removed as dead in the consecutive stages of compiler.
>>>> At any moment, if an error occurs, the node intcon(1) may be 
>>>> subsumed by node intcon(0), therefore  the clone_loop becomes 
>>>> "active" and the original "unfinished in modification" loop will be removed later.
>>>>
>>>> This utility is implemented in class CountedLoopReserveKit.
>>>> The loop cloning and 5 nodes adding is realized in the ctor and 
>>>> possible subsuming intcon(1) by intcon(0) in dtor, so a simple 
>>>> return from the modifying graph function will do graph correction 
>>>> (switching in choice) of the graph.
>>>> Basically, it is sufficient the create a local object of 
>>>> CountedLoopReserveKit class, the scoped dtor makes the choice:
>>>> modified or original loop.
>>>>
>>>> SuperWord::output in this submission is included only to illustrate 
>>>> how to use the CountedLoopReserveKit.
>>>> The actual new SuperWord::output where CountedLoopReserveKit  is 
>>>> indeed substantial is coming in the next patch.
>>>>
>>>> This reserve loop cloning in SuperWord may be disabled by the 
>>>> global flag DoReserveCopyInSuperWord.
>>>>
>>>> -----Original Message-----
>>>> From: Igor Veresov [mailto:igor.veresov at oracle.com]
>>>> Sent: Thursday, September 17, 2015 2:31 PM
>>>> To: hotspot compiler
>>>> Cc: Civlin, Jan
>>>> Subject: RFR(M): 8136725 Provide utility for creation a counted 
>>>> loop reserve copy (clone)
>>>>
>>>> Provide utility for creation a counted loop reserve copy (clone).
>>>> May be used in any graph optimization for simple roll back to the 
>>>> original loop, in partially will be used in SuperWord, where the 
>>>> loop modification goes on-the-fly and potentially may not finish 
>>>> correctly (the patch for SuperWord is coming soon).
>>>>
>>>> This is contributed by Jan Civlin <jan.civlin at intel.com 
>>>> <mailto:jan.civlin at intel.com>>
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~iveresov/loop-reverse/webrev-091515/
>>>>
>>>> igor
>>>>
>>> <webrev-r9100r9092.tar.bz2>
>>