Request for reviews (XS): 7123926: Some CTW test crash: !_control.contains(ctrl)

[Vladimir Kozlov]

I updated webrev. I removed uncast() changes from this fix, we do it later. 
Tested with compiler and java/jang regression tests.

http://cr.openjdk.java.net/~kvn/7123926/webrev.01

Thanks,
Vladimir

Vladimir Kozlov wrote:
> I looked again on my changes and, I think, the lines with 
> push_int();continue; should be also under (arg->outcnt() == 1) 
> condition. If we don't remove call to Integer.toString() - use its 
> result. Otherwise we have to generate conversion from int to String by 
> hand for each use (see int_getChars()). As result Integer.toString() 
> code will not be folded by later optimizations for the case I showed in 
> previous mail. I need to do more testing for this and will resend webrev 
> later.
> 
>  > I'm a little bit worried about this part after seeing your fix for 
> 7181658. Do you think it's safe to use uncast()
> 
> It was another Vladimir (Ivanov) who fixed 7181658 :-)
> 
>  > this way, or should we use eqv_uncast() for comparison at the use 
> sites of StringConcat::argument_uncast()?
> 
> I think using uncast() in argument_uncast() is safe, it is used only in 
> related code. The first time argument_uncast() is called to find a Proj 
> node from SB.toString() call. Then that Proj node passed to merge() 
> method as 'arg'. And in the merge() method argument_uncast() is called 
> second time to compare with that passed 'arg'.
> 
> Thanks,
> Vladimir
> 
> On 7/5/12 8:23 PM, Krystal Mok wrote:
>> Comments inline:
>>
>> On Fri, Jul 6, 2012 at 10:38 AM, Vladimir Kozlov 
>> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>>
>>     Thank you, Kris
>>
>>
>>     Krystal Mok wrote:
>>
>>         Hi Vladimir,
>>
>>         foo(Integer intobj) {
>>            String intstr = intobj.toString();
>>            String str = intstr + ", " + intstr;
>>
>>         What should be the desired behavior for the case above?
>>         Is it all right to skip all Integer.toString() calls, and let 
>> later optimizations handle it?
>>
>>
>>     You have only one toString() call. And it will be kept because it 
>> is referenced by debug info which is copied from
>>     SB constructor to new char[] constructor (AllocateArray node). It 
>> is needed for deoptimization since the bytecode
>>     will be executed after that call.
>>
>> I see. So that's what you meant in the original "Keep case". That 
>> makes sense.
>>
>>     The next code is the case when Integer.toString() will be 
>> eliminated by later optimizations:
>>
>>        StringBuilder sb = new StringBuilder();
>>        String intstr = intobj.toString();
>>        String str = sb.append(intstr).append(__intstr).toString();
>>
>>     I also applied your uncast() suggestion in argument_uncast() method.
>>
>> I'm a little bit worried about this part after seeing your fix for 
>> 7181658. Do you think it's safe to use uncast() this
>> way, or should we use eqv_uncast() for comparison at the use sites of 
>> StringConcat::argument_uncast()?
>>
>> Thanks,
>> Kris
>>
>>     And I fixed typos.
>>
>>     Thanks,
>>     Vladimir
>>
>>
>>         And, two typos:
>>
>>         541               // chain. It's node could be eliminated only 
>> if it's result
>>
>>         It's -> Its. Other occurrences of "it's" should also be 
>> replaced with "its".
>>
>>         543               // An other limitation: it should be used 
>> only once because
>>
>>         An other -> Another.
>>
>>         - Kris
>>
>>         On Fri, Jul 6, 2012 at 4:37 AM, Vladimir Kozlov 
>> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>
>>         <mailto:vladimir.kozlov at __oracle.com 
>> <mailto:vladimir.kozlov at oracle.com>>> wrote:
>>
>>              Vladimir Kozlov wrote:
>>
>>         http://cr.openjdk.java.net/~____kvn/7123926/webrev 
>> <http://cr.openjdk.java.net/~__kvn/7123926/webrev>
>>
>>         <http://cr.openjdk.java.net/~__kvn/7123926/webrev 
>> <http://cr.openjdk.java.net/~kvn/7123926/webrev>>
>>
>>                  7123926: Some CTW test crash: !_control.contains(ctrl)
>>
>>                  Don't try to eliminate Integer::toString() call node 
>> during
>>                  String concatenation optimization if it's result has 
>> several uses.
>>
>>                  Eliminate case:
>>                    foo (Integer intob) {
>>                      String str = "int: " + intobj;
>>
>>                  Keep case:
>>                    foo (Integer intob) {
>>                      String intstr = intobj.toString();
>>                      String str = "int: " + intobj; // second use is 
>> in SB
>>                  allocation
>>
>>
>>              Sorry, it should be: "int: " + intstr;
>>
>>              Vladimir
>>
>>
>>                  debug info
>>
>>                  Tested with failed test.
>>
>>                  Thanks,
>>                  Vladimir
>>
>>
>>