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

[Vladimir Kozlov]

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