Request for reviews (XS): 6866651: Regression: simple int sum crashes jvm (build 1.6.0_14-b08 and 1.7.0-ea-b59)

[Tom Rodriguez]

Looks good.

tom

On Aug 17, 2009, at 10:16 AM, Vladimir Kozlov wrote:

> Looks good.
>
> Vladimir
>
> changpeng fang - Sun Microsystems - Santa Clara United States wrote:
>> http://cr.openjdk.java.net/~cfang/6866651/webrev.01/
>> Ok, updated with  the short cut transformation part removed. Also,
>> for the regression test, we just use "@run main Test" to simplify
>> the Test.
>> Thanks,
>> Changpeng
>> On 08/15/09 11:31, Vladimir Kozlov wrote:
>>> OK. You convinced me.
>>>
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>>
>>>> On Aug 14, 2009, at 5:10 PM, Vladimir Kozlov wrote:
>>>>
>>>>>
>>>>>
>>>>> Tom Rodriguez wrote:
>>>>>> I don't really like this code.  My understanding is that this  
>>>>>> would only occur for nodes that are dead because of CCP.  A  
>>>>>> dead node can only have
>>>>>
>>>>> It is not true. I thought the same before, then I added the  
>>>>> check and hit
>>>>> immediately several cases when the node is alive and its type is  
>>>>> constant:
>>>>
>>>> I saw that those were being handled as well but ignored them for  
>>>> this discussion since they don't seem to motivate this change.   
>>>> They would work just fine without this change.  I don't think  
>>>> short circuiting the transforms of those nodes is worth  
>>>> duplicating the logic.
>>>>
>>>> tom
>>>>
>>>>> - CastII #int:1 after Parse because _gvn.set_type_bottom(ccast) in
>>>>> Parse::adjust_map_after_if().
>>>>> - ConvI2L #long:1 in ConvI2LNode::Ideal() during next transform
>>>>> phase->transform( new (phase->C, 2) ConvI2LNode(y,  
>>>>> TypeLong::make(rylo, ryhi, widen)) )
>>>>> because y = #int:1
>>>>>
>>>>> Vladimir
>>>>>
>>>>>> dead users so doesn't calling add_users_to worklist on a dead  
>>>>>> node seem like a bad idea?  If we want dead nodes from CCP to  
>>>>>> be cleaned up then shouldn't CCP be doing it?
>>>>>> tom
>>>>>> On Aug 14, 2009, at 3:46 PM, Vladimir Kozlov wrote:
>>>>>>> I asked for it to short cut transformations when node's type  
>>>>>>> is known
>>>>>>> const or top. Yes, it is not needed for the bug fix but it could
>>>>>>> reduce time spent in igvn.optimize().
>>>>>>>
>>>>>>> Vladimir
>>>>>>>
>>>>>>> Tom Rodriguez wrote:
>>>>>>>> Why are you including the first part in this fix?  I don't  
>>>>>>>> see the motivation for that change.
>>>>>>>> tom
>>>>>>>> On Aug 14, 2009, at 2:50 PM, changpeng fang - Sun  
>>>>>>>> Microsystems - Santa Clara United States wrote:
>>>>>>>>> http://cr.openjdk.java.net/~cfang/6866651/webrev.00/
>>>>>>>>>
>>>>>>>>> Fixed 6866651: Regression: simple int sum crashes jvm (build  
>>>>>>>>> 1.6.0_14-b08 and 1.7.0-ea-b59)
>>>>>>>>>
>>>>>>>>> Problem:
>>>>>>>>> set_req_X will do dead code elimination if the original  
>>>>>>>>> input has no other use. However, it is possible to
>>>>>>>>> have the current node (this) removed if a dead loop exists.   
>>>>>>>>> So,  dead code elimination in set_req_X is not
>>>>>>>>> safe, and may  cause undesired consequences (like the  
>>>>>>>>> segfault in 6866651)
>>>>>>>>>
>>>>>>>>> Solution:
>>>>>>>>> Instead of doing dead code elimination immediately in  
>>>>>>>>> set_req_X, we put the dead node (original input) into
>>>>>>>>> the worklist, and the dead node will eventually be  
>>>>>>>>> eliminated when it is its turn to be processed.
>>>>>>>>>
>>>>>>>>> In addition,  before ideal transformations, we replace a  
>>>>>>>>> node with a constant if we know it computes a constant
>>>>>>>>> to skip unneeded optimizations on this node. Tests: JPRT,  
>>>>>>>>> CompileTheWorld, Test case in bug report (Test.java in webrev)
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Changpeng
>>>>