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

[Vladimir Kozlov]

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;
   }

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.

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
>