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

[Krystal Mok]

Looks good to me.

- Kris

On Sat, Jul 14, 2012 at 3:46 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com
> wrote:

> 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<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@**oracle.com<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@**oracle.com<vladimir.kozlov at oracle.com>
>>> >
>>>         <mailto:vladimir.kozlov at __orac**le.com <http://oracle.com><mailto:
>>> vladimir.kozlov@**oracle.com <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>
>>> >
>>>
>>>         <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
>>>
>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120714/35af2b0a/attachment.html