[9] RFR(S): 8136469: OptimizeStringConcat fails on pre-sized StringBuilder shapes

[Tobias Hartmann]

Hi Vladimir,

On 07.01.2016 01:35, Vladimir Kozlov wrote:
> Nope. Too much unrelated changes. If you want to go this road - file separate RFE to change phase argument type of Identity() and Value().

Okay, I agree. I filed JDK-8146629 [1].

> And why use PhaseValue and not PhaseGVN as in Ideal()?

Right, we can use PhaseGVN.

> So I agree to do your change in IfNode::Identity(). But as separate fix after general change.

Here is the updated webrev based on JDK-8146629:
http://cr.openjdk.java.net/~thartmann/8136469/webrev.06/

Thanks,
Tobias

[1] https://bugs.openjdk.java.net/browse/JDK-8146629

 
> Thanks,
> Vladimir
> 
> On 1/6/16 3:22 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> I had an off-thread discussion with Roland and we came to the conclusion that all proposed fixes essentially work around the fact that we are unable to determine if Identity is called from GVN or IGVN. As Roland pointed out, we would probably miss to adapt such a fix if we ever get the ability to check for GVN/IGVN.
>>
>> Here is a more robust solution not depending on any worklist ordering assumptions and not causing unexpected side effects:
>> Since Node::Identity(PhaseTransform* phase) is always called with either PhaseGVN or PhaseIterGVN, we can change the argument to type PhaseValues* and can therefore simply use phase->is_IterGVN() to determine if we were called from GVN or IGVN. This could also be useful for other changes. Of course, this introduces an additional virtual call but we are already calling phase->is_IterGVN() at many other places in the code. In the future, these calls could be replaced by a field access (as Vladimir suggested in the RFR for 8139771).
>>
>> http://cr.openjdk.java.net/~thartmann/8136469/webrev.05/
>>
>> What do you think?
>>
>> Thanks,
>> Tobias
>>
>>
>> On 18.09.2015 11:57, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8136469
>>> http://cr.openjdk.java.net/~thartmann/8136469/webrev.00/
>>>
>>> Problem:
>>> When creating a pre-sized StringBuilder, C2's string concatenation optimization sometimes fails to optimize the chain (see [1]). The problem is that the initial size of the StringBuilder depends on a static final boolean that is initialized to true at runtime. Therefore the string concatenation control flow chain [2] contains an IfNode with a ConI (1) as input instead of the expected BoolNode and StringConcat::validate_control_flow() silently bails out.
>>>
>>> Solution:
>>> I changed the implementation to skip dead tests as they would be removed by IGVN later anyway. I added an assert to make sure we don't bail out silently if the input of the IfNode is not a bool. I also had to change validate_mem_flow() to handle dead ifs. Further, the assert in line 825 is unnecessary because we execute the same check in as_If().
>>>
>>> Testing:
>>> - New test (TestPresizedStringBuilder)
>>> - JPRT
>>>
>>> Thanks,
>>> Tobias
>>>
>>> [1] https://bugs.openjdk.java.net/secure/attachment/53220/TestPresizedStringBuilder.java
>>> [2] https://bugs.openjdk.java.net/secure/attachment/53218/graph.png
>>>