RFR(M): 8027422: assert(_gvn.type(obj)->higher_equal(tjp)) failed: cast_up is no longer needed

[Christian Thalinger]

Yes, much better.  Thank you.
On Jan 20, 2014, at 2:09 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

>> Very good.
>> 
>> You need to use 2014 Copyright year in the test (no need for re-review).
> 
> Thanks, Vladimir.
> 
> What about you, Chris? Do you think it’s ok now?
> 
> Roland.
> 
>> 
>> Thanks,
>> Vladimir
>> 
>> On 1/17/14 2:19 AM, Roland Westrelin wrote:
>>> What about this?
>>> 
>>> http://cr.openjdk.java.net/~roland/8027422/webrev.03/
>>> 
>>> Roland.
>>> 
>>> 
>>> On Jan 16, 2014, at 10:11 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>> 
>>>> On 1/16/14 1:05 AM, Roland Westrelin wrote:
>>>>> 
>>>>> Thanks for reviewing this, Christian.
>>>>> 
>>>>>> Seeing all these:
>>>>>> 
>>>>>> true /* include_speculative */
>>>>>> 
>>>>>> I wonder if we should add new methods for these.  It would make it easier to see the users of the speculative versions.
>>>>> 
>>>>> A new meet_speculative() method?
>>>>> I’m ok with it. Vladimir, what do you think?
>>>> 
>>>> I was going to suggest it too but then you need additional methods for other methods: join(), higher_equal(), filter(). So I am fine with it if you do all of them.
>>>> 
>>>> Vladimir
>>>> 
>>>>> 
>>>>> Roland.
>>>>> 
>>>>>> 
>>>>>> On Jan 14, 2014, at 7:25 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>>> 
>>>>>>> Looks good to me.
>>>>>>> 
>>>>>>> On 1/14/14 1:13 AM, Roland Westrelin wrote:
>>>>>>>> http://cr.openjdk.java.net/~roland/8027422/webrev.02/
>>>>>>>> 
>>>>>>>> I fixed the verification code at the end of Compile::remove_speculative_types(), used this format everywhere:
>>>>>>>> t->filter(_type, true /* include_speculative */);
>>>>>>>> 
>>>>>>>> renamed NodeHash::check_speculative_types() to NodeHash::check_no_speculative_types(), added PhaseIterGVN::check_no_speculative_types() and now call it from the verification code of Compile::remove_speculative_types().
>>>>>>>> 
>>>>>>>> Do I need more than 1 review for this?
>>>>>>> 
>>>>>>> Yes, you need an other review for this change. Ask Chris or Igor.
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>> 
>>>>>>>> 
>>>>>>>> Roland.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Jan 13, 2014, at 11:03 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>>>>> 
>>>>>>>>>> compile.cpp: new verification code (under ASSERT) at the end of remove_speculative_types() checks only the root node - nothing else is pushed on worklist. Also add comment to this new code.
>>>>>>>>> 
>>>>>>>>> Thanks for catching that.
>>>>>>>>> Is:
>>>>>>>>> // Verify that after the IGVN is over no speculative type has resurfaced
>>>>>>>>> good as a comment?
>>>>>>>>> 
>>>>>>>>>> Passing 'true' parameter is not very informative. You can use local variable:
>>>>>>>>>> 
>>>>>>>>>> bool include_speculative = true;
>>>>>>>>>> t->filter(_type, include_speculative);
>>>>>>>>>> 
>>>>>>>>>> An other way to make code more informative is to add a comment to parameter:
>>>>>>>>>> 
>>>>>>>>>> t->filter(_type, true /* include_speculative */);
>>>>>>>>>> 
>>>>>>>>>> Either way is fine.
>>>>>>>>> 
>>>>>>>>> Will do one of these.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Roland.
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Vladimir
>>>>>>>>>> 
>>>>>>>>>> On 1/10/14 1:46 AM, Roland Westrelin wrote:
>>>>>>>>>>> Hi Vladimir,
>>>>>>>>>>> 
>>>>>>>>>>> Here is a new webrev for this:
>>>>>>>>>>> http://cr.openjdk.java.net/~roland/8027422/webrev.01/
>>>>>>>>>>> 
>>>>>>>>>>> I fixed the issues you mentioned in your review.
>>>>>>>>>>> I added a call to remove_speculative() to the ConNode constructor. When a node becomes constant, its speculative part can be not null. The IGVN doesn’t kill ConNodes so without a call to remove_speculative() a ConNode with a speculative part can sneak past the call to Compile::remove_speculative_types().
>>>>>>>>>>> I also added a verification method:
>>>>>>>>>>> NodeHash::check_speculative_types()
>>>>>>>>>>> to check that no TypeNode with a speculative type is still in the IGVN hash table after Compile::remove_speculative_types()
>>>>>>>>>>> 
>>>>>>>>>>> Roland.
>>>>>>>>>>> 
>>>>>>>>>>> On Nov 19, 2013, at 8:49 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> On 11/19/13 11:45 AM, Roland Westrelin wrote:
>>>>>>>>>>>>> Thanks for reviewing this, Vladimir.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> On Nov 19, 2013, at 1:34 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Next 2 places in type.cpp pass 'true' to meet() unconditionally:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 1929     return TypeAry::make(_elem->meet(a->_elem, true),
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 3812     const TypeAry *tary = _ary->meet(tap->_ary, true)->is_ary();
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Should TypeAryPtr::remove_speculative() also clean _speculative in element's type?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> You’re right. It probably should.
>>>>>>>>>>>>> So I need to add a remove_speculative() method to TypeAry. Then the 2 places where true is passed to meet() for TypeAry don’t matter anymore because remove_speculative() is called from meet() and remove_speculative now has an effect on TypeAry, right?
>>>>>>>>>>>> 
>>>>>>>>>>>> Right, passing 'true' will work in all cases then.
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Vladimir
>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Could you make printing code with 'this_t' aligned again in Type::meet()?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Sure.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Roland.
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> thanks,
>>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> On 11/18/13 1:15 PM, Roland Westrelin wrote:
>>>>>>>>>>>>>>> The root of the problem is that during the null check when the type of obj is improved in GraphKit::cast_not_null():
>>>>>>>>>>>>>>> const Type *t_not_null = t->join(TypePtr::NOTNULL, true);
>>>>>>>>>>>>>>> The join with TypePtr::NOTNULL is not applied to the speculative part. In fact, no meet between a TypeOopPtr and a TypePtr modifies the speculative part. One way to fix it would be to apply the meet with a TypePtr to the speculative part as well as the standard part of the type which I tried: then we need to move the _speculative field up in TypePtr and modify all operations on TypePtr to operate on _speculative so that the type system remains symmetric.
>>>>>>>>>>>>>>> In many places where we mix a TypePtr with a TypeOopPtr we actually don’t care about the speculative part. I changed the following operations on Type:
>>>>>>>>>>>>>>> higher_equal()
>>>>>>>>>>>>>>> meet()
>>>>>>>>>>>>>>> join()
>>>>>>>>>>>>>>> filter()
>>>>>>>>>>>>>>> so that by default they don’t return a result that include the speculative part of the type. Where we need the speculative part of the type, we have to explicitly request it.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> I also fixed a problem with Type nodes with a _type of TypeNarrowOop that wouldn’t drop the speculative part of the type during Compile::remove_speculative_types().
>>>>>>>>>>>>>>> I included small clean ups that Mikael suggested privately (dropped the duplicate check for res->isa_oopptr() in TypeOopPtr::meet, make remove_speculative not go through the exercise of creating a new type if speculative is NULL).
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~roland/8027422/webrev.00/
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Roland.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>>> 
>>> 
>