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

[Vladimir Kozlov]

Thanks, Kris

Vladimir

On 7/13/12 9:05 PM, Krystal Mok wrote:
> Looks good to me.
>
> - Kris
>
> On Sat, Jul 14, 2012 at 3:46 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com <mailto: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 at oracle.com> <mailto: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>>
>             <mailto:vladimir.kozlov@ <mailto:vladimir.kozlov@>__orac__le.com <http://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>>
>
>             <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
>
>
>
>