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

[Roland Westrelin]

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