review (M) for 6892658: C2 should optimize some stringbuilder patterns

[Tom Rodriguez]

On Nov 10, 2009, at 2:11 PM, Vladimir Kozlov wrote:

> Tom,
> 
> here is review of all files but 2 new, I am looking on them next
> 
> the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string

I was trying to mimic Integer.MIN_VALUE.  If you want int in there then how about the_min_jint_string?

> universe.hpp
> static oop _the_MIN_VALUE_string;  // A cache of "" as a Java string
>                                                  ^ "-2147483648"

Fixed.

> c2_globals.hpp
> I thought we will use experimental for OptimizeStringConcat.

I guess we could.  I don't really like experimental much but I'm not completely against it.

> callGenerator.cpp
> missing _call_node(NULL) in DirectCallGenerator() constructor.
> 
>  bool _separate_io_proj; <- add field's descriptor by copying the comment
>                             from LateInlineCallGenerator::generate()

Ok.

> 
> do_late_inline()
> add checks to not inline when
> call_node()->in(0) == NULL || call_node()->in(0)->is_top()

Ok

> 
> +   for (uint i1 = 0; i1 < call->req(); i1++) {
>                           ^ size

Ok.

> callnode.cpp
> You copied code from PhaseMacroExpand::extract_call_projections().
> Can you use your new method in macro.cpp also? At least for HS17 changes
> when you will have more time.

I'll make that change later.

> compile.cpp
> In gvn_replace_by() why there is no initial_gvn()->hash_insert(use)?

It was an oversight.  I've added it.  The code should roughly mirror Node::subsume_node.

> Instead of OptimizeStringConcat && has_stringbuilder() and OptimizeStringConcat
> checks may be we can use only has_stringbuilder() (or different name for query method)
> and check OptimizeStringConcat in parseHelper.cpp where has_stringbuilder is set?

I'd be ok with that.

> 
> doCall.cpp
> I saw cases when append methods were not inlined because they were
> already compiled into "big" compiled method. You call for_late_inline()
> after ok_to_inline(), so may be we should relax that condition for
> OptimizeStringConcat case.

The code doesn't care whether the interesting methods are inlined or not.  If the methods would have been inlined then we register a late inline for them.  If they wouldn't have been inlined then we tell the DirectCallGenerator to use separate io projs so that we can properly find all the edges if we need to replace it.  THe late inlining logic is mainly about giving us the same result if we don't end up performing the optimization.

> Should you check that safepoint has > jvms->argoff() inputs?:
> Node* receiver = jvms->map()->in(jvms->argoff() + 1);

I guess I could but if there aren't enough then the call itself is malformed and we'll die later won't we?  Do you think i should?

> 
> macro.cpp
> Can you print array only when it is array?:
> +     log->head("eliminate_allocation %s type='%d'",
> +               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));
> 
> the same with "lock":"unlock"

I could but that's not very good xml.  I think I'll just leave that out since arrayness should be discerned from the klass.  I'd prefer to leave it as it for lock though.

tom

> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/