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

[changpeng fang - Sun Microsystems - Santa Clara United States]

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