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

[Vladimir Kozlov]

Yes, webrev.04  is no go. I was referring webrev.03 but I missed that it does not have can_reshape.

Vladimir

On 1/5/16 9:13 AM, Tobias Hartmann wrote:
>
> On 05.01.2016 18:05, Vladimir Kozlov wrote:
>> On 1/4/16 11:58 PM, Tobias Hartmann wrote:
>>> Hi Vladimir,
>>>
>>> thanks for the review.
>>>
>>> On 05.01.2016 00:52, Vladimir Kozlov wrote:
>>>> On 1/4/16 3:35 AM, Tobias Hartmann wrote:
>>>>> Hi Roland,
>>>>>
>>>>> sorry for the delay.
>>>>>
>>>>> On 07.10.2015 11:06, Roland Westrelin wrote:
>>>>>>>> Maybe we could add an IfProjNode::Ideal method that disconnects the other branch of the If when this branch is always taken and that does so even during parsing. Given Ideal is called before Identity, that would guarantee the next call to Identity optimizes the If out.
>>>>>>>
>>>>>>> As you suggested, I added an IfProjNode::Ideal that disconnects the never taken branch from the IfNode. The subsequent call to Identity then removes the IfNode:
>>>>>>> http://cr.openjdk.java.net/~thartmann/8136469/webrev.03/
>>>>>>>
>>>>>>> However, I wondered if this is "legal" because the comment in Node::ideal says:
>>>>>>>
>>>>>>> // The Ideal call almost arbitrarily reshape the graph rooted at the 'this'
>>>>>>> // pointer.
>>>>>>>
>>>>>>> But we are changing the graph "above" the this pointer. I executed tests with -XX:+VerifyIterativeGVN and everything seems to work fine.
>>>>>>> Another solution would be to cut the *current* branch if it is never taken:
>>>>>>> http://cr.openjdk.java.net/~thartmann/8136469/webrev.02/
>>>>>>>
>>>>>>> But this solution depends on the assumption that we execute the identity() of the other ProjNode which is not guaranteed by GVN (I think).
>>>>>>>
>>>>>>> Therefore I would like to go for webrev.03. I verified that this solves the problem and tested the fix with JPRT.
>>>>>>
>>>>>> I thought about this more and I don’t think either work ok.
>>>>>>
>>>>>> The problem with webrev.02 is that depending on the order the projection nodes are allocated and transformed, the optimization may not happened:
>>>>>>
>>>>>> Node* never_taken = new IfTrueNode(..);
>>>>>> Node* always_taken = new IfFalseNode(..);
>>>>>> always_taken = gvn.transform(always_taken);
>>>>>> never_taken = gvn.transform(never_taken);
>>>>>>
>>>>>> The problem with webrev.03 is that we may change a node that is not yet transformed (never_taken changed by call to gvn.transform(always_taken)). Not sure if it could break existing code but it’s clearly an unexpected behavior.
>>>>>
>>>>> Right, that could be a problem.
>>>>
>>>> I don't see a problem. But IfProjNode::Ideal() should have additional checks for that:
>>>>
>>>>     // Check for dead control input
>>>>     if (in(0) && remove_dead_region(phase, can_reshape)) {
>>>>       return this;
>>>>     }
>>>>     // Don't bother trying to transform a dead node
>>>>     if (in(0) && in(0)->is_top()) {
>>>>       return NULL;
>>>>     }
>>>
>>> Right, I'll add those.
>>>
>>>> Also instead of set_req() use:
>>>>
>>>> PhaseIterGVN* igvn = phase->is_IterGVN();
>>>> igvn->replace_input_of(other, 0, phase->C->top());
>>>>
>>>> This way following gvn.transform(never_taken); will work fine.
>>>
>>> But this assumes that we are only executing the code with IGVN but we also want to cut off the dead branch with GVN. Or am I missing something?
>>
>> webrev.04 checks can_reshape which is true only with IGVN.
>> For GVN you can do it by hand:
>>
>>          bool is_in_table = C->initial_gvn()->hash_delete(other);
>>          other->set_req(0, phase->C->top());
>>          if (is_in_table) {
>>            C->initial_gvn()->hash_find_insert(other);
>>          }
>>          C->record_for_igvn(other);
>>
>> Note, during Parse (GVN) we don't remove dead code aggressively.
>
> Right, I thought you were referring to Roland's comment about webrev.03.
>
> As I wrote in a previous email, I'm afraid that the webrev.04 solution re-introduces JDK-8027626. If we call IfProjNode::Identity() during GVN and replace the ProjNode by If's input, we end up with a node having two control outputs until we remove the dead branch during IGVN. Do you think that isn't a problem?
>
> Thanks,
> Tobias
>
>>
>> Vladimir
>>
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>>> An other way would be to remove the in(0)->outcnt() == 1 check from IfProjNode::Identity() and in an IfProjNode::Ideal method do what you do in webrev.03 but when can_reshape is true only.
>>>>>
>>>>> Here is the new webrev:
>>>>> http://cr.openjdk.java.net/~thartmann/8136469/webrev.04/
>>>>>
>>>>> However, I'm afraid that this re-introduces JDK-8027626. If we call IfProjNode::Identity() during GVN and replace the ProjNode by If's input, we end up with a node having two control outputs until we remove the dead branch during IGVN. Right?
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>