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

[Vladimir Kozlov]

Tom Rodriguez wrote:
> 
>> Also _begin instead of other->_begin
>> in result->set_allocation(other->_begin)
> 
> _begin is must be the earliest JVMState of the pattern and other->_begin has to be earlier than _begin otherwise the couldn't be merged so I can't just swap them around.
> 

Then I don't get how you optimize next code:

SB.append((new SB()).append(s).toString()).toString()

and I don't see any checks that other->_begin dominates _begin.

> 
>> There are several places where you do next check,
>> may be you can factor it in a separate function:
>>
>> method->holder() == C->env()->StringBuilder_klass() ||
>> method->holder() == C->env()->StringBuffer_klass()
> 
> I'm not sure factoring it out would be better.

OK.

> 
>> May be also verify has_stringbuilder() in PhaseStringOpts().
> 
> Why?

OK, I see that caller code of PhaseStringOpts() has has_stringbuilder()

>> Also this coalesce code will not work if "other" is used by
>> several sc/arguments since you removed it from the list after
>> first match and merge. For example:
>>
>> String s0 = new SB().append(1)...toString();
>> String s1 = new SB().append(s0).append(s0).toString();
>>
>> I would keep it and always replace "c" with merged
>> (you need to modify StringConcat::merge() as I pointed above).
>> The "o" will be removed automatically if there are no other uses.
> 
> I don't want to support that.  I don't think that's an interesting pattern.  It would also require rewriting the management of the control and trap lists and I don't want to get into that.
> 

OK.

Vladimir

>> I will look on copy_string() and related methods tomorrow.
> 
> Thanks.
> 
> tom
> 
>> Vladimir
>