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

[Vladimir Kozlov]

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