[9] RFR (XS): 8036667: "assert(adr->is_AddP() && adr->in(AddPNode::Offset)->is_Con()) failed: offset is a constant" with FoldStableValues on

[Vladimir Ivanov]

Thanks, Vladimir.

Best regards,
Vladimir Ivanov

On 3/6/14 9:47 PM, Vladimir Kozlov wrote:
> Looks good.
>
> Vladimir
>
> On 3/6/14 9:40 AM, Vladimir Ivanov wrote:
>> Unfortunately, replacing tp->offset() with
>> AddPNode::Ideal_base_and_offset doesn't work for other cases in
>> LoadNode::Value. I have to revert to using tp->offset().
>>
>> Updated webrev: http://cr.openjdk.java.net/~vlivanov/8036667/webrev.02/
>>
>> I did some testing and haven't found any problems with the adr being
>> PhiNode, but I'm not sure I understand all aspects
>> of this code. So I decided to skip phi case by adding adr->is_AddP()
>> case. Some other cases in LoadNode::Value have
>> similar check.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 3/6/14 6:01 AM, Vladimir Ivanov wrote:
>>>> Why you did changes in extract_uncommon_trap_request()? How we can get
>>>> phi there?
>>> It's a fix for a crash w/ -XX:+TraceIterGVN.
>>> Will file a separate bug to track it, as you suggested.
>>>
>>>> What is 'off' value in this case? How you can get constant value
>>>> from C2
>>>> type system (tp->offset()) but not from looking on ideal nodes?
>>> Good point! My fix isn't correct in some cases.
>>>
>>> In case only 1 phi input has non-top type, it should be safe to use
>>> offset. But when at least 2 inputs are live, it's not correct (same
>>> offset, but different address).
>>>
>>> So, I need to ignore phi case completely.
>>>
>>> Updated fix: http://cr.openjdk.java.net/~vlivanov/8036667/webrev.01/
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>>
>>>> On 3/5/14 4:26 PM, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/8036667/webrev.00/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8036667
>>>>> 10 lines changed: 8 ins; 0 del; 2 mod
>>>>>
>>>>> The assert is too strong. It doesn't take into account the case when
>>>>> constant offset comes from phi node.
>>>>>
>>>>> The fix is to relax the assertion by skipping phi node case. I decided
>>>>> to keep it simple and avoided doing deep verification of the graph.
>>>>>
>>>>> Also, fixed a crash w/ -XX:+TraceIterGVN (see callnode.cpp).
>>>>>
>>>>> Testing: failing tests.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov